Re: Collation versioning - Mailing list pgsql-hackers

On Tue, Mar 17, 2020 at 11:42:34AM +0100, Julien Rouhaud wrote:
> On Tue, Mar 17, 2020 at 08:19:30AM +0100, Julien Rouhaud wrote:
>> On Tue, Mar 17, 2020 at 03:37:49PM +0900, Michael Paquier wrote:
>>> It seems to me that you could add an extra test with a catalog that
>>> does not exist, making sure that NULL is returned:
>>> SELECT to_regtype('ng_catalog."POSIX"');
>>
>>
>> Agreed, I'll add that, but using a name that looks less like a typo :)
>
> Tests added, including one with an error output, as the not existing schema
> doesn't reveal the encoding.

Yep.

>> Ah good catch, I missed that during the NameData/text refactoring.  I'll fix it
>> anyway, better to have clean history.
>
> And this should be fixed too.

Thanks.

It would be good to be careful about the indentation.  Certain parts
of 0003 don't respect the core indentation.  Not necessarily your job
though.  Other than that 0003 seems to be in good shape.

@@ -54,6 +55,7 @@ recordDependencyOn(const ObjectAddress *depender,
 void
 recordMultipleDependencies(const ObjectAddress *depender,
                            const ObjectAddress *referenced,
+                           const char *version,
                            int nreferenced,
                            DependencyType behavior)
Nit from patch 0002: the argument "version" should be fourth I think,
keeping the number of referenced objects and the referenced objects
close.  And actually, this "version" argument is removed in patch
0004, replaced by the boolean track_version.  (By reading the
arguments below I'd rather keep *version).

So, 0004 is the core of the changes.  I have found a bug with the
handling of refobjversion and pg_depend entries.  When swapping the
dependencies of the old and new indexes in index_concurrently_swap(),
refobjversion remains set to the value of the old index.  I used a
manual UPDATE on pg_depend to emulate that with a past fake version
string to emulate that (sneaky I know), but you would get the same
behavior with an upgraded instance.  refobjversion should be updated
to the version of the new index.

+void recordDependencyOnCollations(ObjectAddress *myself,
+                                  List *collations,
+                                  bool record_version)
Incorrect declaration format.

+        if (track_version)
+        {
+            /* Only dependency on a collation is supported. */
+            if (referenced->classId == CollationRelationId)
+            {
+                /* C and POSIX collations don't require tracking the version */
+                if (referenced->objectId == C_COLLATION_OID ||
+                    referenced->objectId == POSIX_COLLATION_OID)
+                    continue;
I don't think that the API is right for this stuff, as you introduce
collation-level knowledge into something which has been much more
flexible until now.  Wouldn't it be better to move the refobjversion
string directly into ObjectAddress?

+ * Test if a record exists for the given depender and referenceds addresses.
[...]
+           /* recordDependencyOnSingleRelExpr get rid of duplicate entries */
Typos.

+   /* XXX should we warn about "disappearing" versions? */
+   if (current_version)
What are disappearing version strings?

+    /*
+     * Perform version sanity checks on the relation underlying indexes if
+     * it's not a VACUUM FULL
+     */
+    if (!(options & VACOPT_FULL) && onerel && !IsSystemRelation(onerel) &&
+            onerel->rd_rel->relhasindex)
Should this explain why?

+            /* Record collations from the type itself, or underlying in case of
+             * complex type.  Note that if the direct parent is a CollateExpr
+             * node, there's no need to record the type underlying collation if
Comment block format.

+-- for key columns, hash indexes should record dependency on the collation but
+-- not the version
+CREATE INDEX icuidx18_hash_d_es ON collate_test USING hash (d_es);
Why is that?  The code in 0004 has no mention of that, and relies on
this code path:
+/*
+ * Returns whether the given index access method depend on a stable collation
+ * order.
+ */
+static bool
+index_depends_stable_coll_order(Oid amoid)
+{
+   return (amoid != HASH_AM_OID &&
+           strcmp(get_am_name(amoid), "bloom") != 0);
+}
And how is that even extensible for custom index AMs?  There are other
things than bloom out there.

+   /*
+    * We only care about dependencies on a specific collation if a valid Oid
+    * was given.=
+    */
[...]
+       /*
+        * do not issue UNKNOWN VERSION is caller specified that those are
+        * compatible
+        */
Typos from patch 5.

-           $self->logfile, '-o', "--cluster-name=$name", 'start');
+           $self->logfile, '-o', $options, 'start');
This needs to actually be shaped with two separate arguments for
--cluster-name or using quotes would not work properly if I recall
correctly.  Not your patch's fault, so I would fix that separately.
--
Michael