Re: Collation versioning - Mailing list pgsql-hackers

Hi Michael,

On Fri, Aug 14, 2020 at 04:37:46PM +0900, Michael Paquier wrote:
> On Fri, Aug 14, 2020 at 02:21:58PM +1200, Thomas Munro wrote:
> > Thanks Julien.  I'm planning to do a bit more testing and review, and
> > then hopefully commit this next week.  If anyone else has objections
> > to this design, now would be a good time to speak up.
> 
> The design to use pg_depend for the version string and rely on an
> unknown state for indexes whose collations are unknown has a clear
> consensus, so nothing to say about that.  It looks like this will
> benefit from using multi-INSERTs with pg_depend, actually.
> 
> I have read through the patch, and there are a couple of portions that
> could be improved and/or simplified.
> 
>  /*
> - * Adjust all dependency records to come from a different object of the same type
>   * Swap all dependencies of and on the old index to the new one, and
> + * vice-versa, while preserving any referenced version for the original owners.
> + * Note that a call to CommandCounterIncrement() would cause duplicate entries
> + * in pg_depend, so this should not be done.
> + */
> +void
> +swapDependencies(Oid classId, Oid firstObjectId, Oid secondObjectId)
> +{
> +   changeDependenciesOf(classId, firstObjectId, secondObjectId, true);
> +   changeDependenciesOn(classId, firstObjectId, secondObjectId);
> +
> +   changeDependenciesOf(classId, secondObjectId, firstObjectId, true);
> +   changeDependenciesOn(classId, secondObjectId, firstObjectId);
> +}
> 
> The comment on top of the routine is wrong, as it could apply to
> something else than indexes.  Anyway, I don't think there is much
> value in adding this API as the only part where this counts is
> relation swapping for reindex concurrently.  It could also be possible
> that this breaks some extension code by making those static to
> pg_depend.c.

It seemed cleaner but ok, fixed.

> 
> -long
> +static long
>  changeDependenciesOf(Oid classId, Oid oldObjectId,
> -                    Oid newObjectId)
> +                    Oid newObjectId, bool preserve_version)
> All the callers of changeDependenciesOf() set the new argument to
> true, making the false path dead, even if it just implies that the
> argument is null.  I would suggest to keep the original function
> signature.  If somebody needs a version where they don't want to
> preserve the version, it could just be added later.

Fixed.

> 
> +                * We don't want to record redundant depedencies that are used
> +                * to track versions to avoid redundant warnings in case of
> s/depedencies/dependencies/
> 
> +       /*
> +        * XXX For deterministic transaction, se should only track the
> version
> +        * if the AM relies on a stable ordering.
> +        */
> +       if (determ_colls)
> +       {
> +           /* XXX check if the AM relies on a stable ordering */
> +           recordDependencyOnCollations(&myself, determ_colls, true);
> Some cleanup needed here?  Wouldn't it be better to address the issues
> with stable ordering first?

Didn't we just agreed 3 mails ago to *not* take care of that in this patch, and
add an extensible solution for that later?  I kept the XXX comment to make it
extra clear that this will be addressed.

> 
> +           /* recordDependencyOnSingleRelExpr get rid of duplicated
> entries */
> s/get/gets/, incorrect grammar.

Fixed.

> 
> +   /* XXX should we warn about "disappearing" versions? */
> +   if (current_version)
> +   {
> Something to do here?

I'm not sure.  This comment is to remind that we won't warn that an index
might get broken if say gnu_get_libc_version() stop giving a version number at
some point.  I don't think that this will happen, but just in case there's a
comment to keep it in mind.

> +       /*
> +        * We now support versioning for the underlying collation library on
> +        * this system, or previous version is unknown.
> +        */
> +       if (!version || (strcmp(version, "") == 0 && strcmp(current_version,
> +                                                           "") != 0))
> Strange diff format here.

That's what pgindent has been doing for some time, ie. indent at the same level
of the opening parenthesis.

> 
> +static char *
> +index_check_collation_version(const ObjectAddress *otherObject,
> +                             const char *version,
> +                             void *userdata)
> All the new functions in index.c should have more documentation and
> comments to explain what they do.

Fixed.

> 
> +   foreach(lc, collations)
> +   {
> +       ObjectAddress referenced;
> +
> +       ObjectAddressSet(referenced, CollationRelationId, lfirst_oid(lc));
> +
> +       recordMultipleDependencies(myself, &referenced, 1,
> +                                  DEPENDENCY_NORMAL, record_version);
> +   }
> I think that you could just use an array of ObjectAddresses here, fill
> in a set of ObjectAddress objects and just call
> recordMultipleDependencies() for all of them?  Just create a set using
> new_object_addresses(), register them with add_exact_object_address(),
> and then finish the job with record_object_address_dependencies().

Fixed.