Re: Collation versioning

On Tue, Oct 27, 2020 at 1:34 AM Julien Rouhaud <rjuju123@gmail.com> wrote:
> On Sun, Oct 25, 2020 at 7:13 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
> > I didn't review all the changes yet, so I'll probably post a deeper
> > review tomorrow.  I'm not opposed to this new approach, as it indeed
> > saves a lot of code.  However, looking at
> > do_collation_version_check(), it seems that you're saving the
> > collation in context->checked_calls even if it didn't raise a WARNING.
> > Since you can now have indexes with dependencies on a same collation
> > with both version tracked and untracked (see for instance
> > icuidx00_val_pattern_where in the regression tests), can't this hide
> > corruption warning reports if the untracked version is found first?
> > That can be easily fixed, so no objection to that approach of course.

Right, fixed.

> I finish looking at the rest of the patches.  I don't have much to
> say, it all looks good and I quite like how much useless code you got
> rid of!

Thanks!  I tested a bunch of permutations[1] of cross-version
pg_update, with and without ICU, with and without libc version
support, and fixed some problems I found in pg_dump:

1.  We need to print OIDs as %u, not %d.  Also, let's use
'%u'::pg_catalog.oid to be consistent with nearby things.

2.  We dump binary_upgrade_set_index_coll_version(<index>, NULL, ...)
to blow away the new cluster's versions before we import the old
versions.  OK, but the function was marked STRICT...

3.  We dump binary_upgrade_set_index_coll_version(<index>,
<collation>, <version>), to import the old cluster's version, where
<collation> is an OID.  OK, but we need the new cluster's OID, not the
old one, so it needs to be an expression like
'pg_catalog."fr_FR"'::regcollation (compare the other references to
collations in the dump, which are all by name).

4.  I didn't really like the use of '' for unknown.  I figured out how
to use NULL for that.

[1] https://github.com/macdice/some_pg_upgrade_tests