Re: Collation versioning

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.

-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.

+ * 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?

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

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

+ /*
+ * 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.

+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.

+ 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().
--
Michael