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
signature.asc
Description: PGP signature