Re: Collation versioning

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.