out of scope, i should have pushed this before creating the diff.

This is the constraint which permits to use the upsert scheme in the swh_content_add.

Same for upsert scheme used in swh_skipped_content_add.

It's not null here for now since we need to run the computations on that column for the existing hash.

same here

This is a "primary key" check. It must not change until all the columns are not null, else the equality will fail and we'll get duplicate entries: the old entries with only the old checksums will not match newer entries with the new checksum.

Unfortunately, for now, this check is not equivalent to the previous one.

And neither is this one.

No it's not. on conflict (foo, bar, baz, quux) will do something if any constraint on columns foo, bar, baz or quux fails.

As each checksum column has a unique index, those will always hit before the complex constraint.

The same remark applies to this constraint as well.

This whole logic seems buggy. Why would sha1_git be "more important" than other checksums here?

This should get factored out in db. (db.Db.content_keys?)

Same here (db.Db.skipped_content_keys?)

Yes indeed!
This can be installed once the migration is complete (through the rehash routine is deployed and ran).
That is once that the blake2s256 (or blake2b512) is constrained to not be null.

(In a future 105 version for example).

Yes, this goes in 105 version as well.

Yes, this goes in 105 version as well.

No it's not. on conflict (foo, bar, baz, quux) will do something if any constraint on columns foo, bar, baz or quux fails.

So, if i understood you right (and the doc), i shall update the swh_content_add and swh_skipped_content_add with the 'on conflict do update' (in the 105.sql script).

As each checksum column has a unique index,

well no longer all columns since we will drop unicity constraint on sha256 and blake2s256 (as per mentioned above).

As each checksum column has a unique index, those will always hit before the complex constraint.

Thus, again, rewriting the queries to add to on conflict do nothing seems appropriate.

db.Db.content_get_metadata_keys already holds the same list.

right.

Thus, again, rewriting the queries to add to on conflict do nothing seems appropriate.

If this was possible though, which i doubt...

Reading again the documentation...
What I understand is that we can only put one 'on conflict' clause either on one constraint, or index, ...

Also if i remember right, having directly one 'on conflict do nothing' (without specifying anything), does not work.
I remember posgresql asking for a specific constraint/index/etc... to act upon.

Anyway, moving on, that point is, for now, out of scope.

I don't know the initial reason. I assumed it was to be able to use sets.

I kept that set logic but use an aggregate key of all hashes (in a specific order).
This is then used later (on line 144) to check the existence of such skipped content.

This won't work if one of the values is None, which might happen (the git loader currently computes all checksums for contents that are too big, but nothing imposes it). That constraint should also be reflected in tests.

It should work if you make the "unique key" a tuple of values (return tuple(values) instead of b''.join(values)).

bug coalesce(blake2s256, '')

bug coalesce(blake2s256, '')

This won't work if one of the values is None, which might happen (the git loader currently computes all checksums for contents that are too big, but nothing imposes it).

Oh, right!

That constraint should also be reflected in tests.

Yes, indeed, adding the case in test make it break!
Nice catch.

It should work if you make the "unique key" a tuple of values (return tuple(values) instead of b''.join(values)).

Right. I did not know we could use tuple in set, awesome thanks. That's way clearer.

And \o/

This also had the fortunate effect to show a reverted bug fix in my sql.
I already fixed this in the previous manually crafted 104.sql.
But then, it was missing in swh-func.sql...

(cf. swh_skipped_content_add function...)