Re: Collation versioning - Mailing list pgsql-hackers

On Wed, Mar 18, 2020 at 04:55:25PM +0900, Michael Paquier wrote:
> 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:
>
> 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.


I'll try to do a partial pgindent run on all patches before next patchset.


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


Oh good catch.  I'll dig into it.


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


We could, but we would then need to add code to retrieve the collation version
in multiple places (at least RecordDependencyOnCollation and
recordDependencyOnSingleRelExpr).  I'm afraid that'll open room for bugs if
some other places are missed, now or later, even more if more objects get a
versionning support.


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


A collation for which a version was previously recorded but that now doesn't
report a version anymore.  For instance if upgrading from glibc X.Y to X.Z
changes gnu_get_libc_version() to return NULL, or if a new major pg version
removes support for glibc (or other lib) versioning.  It seems unlikely to
happen, and if that happens there's nothing we can do anymore to warn about
possible corruption anyway.


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


I was assuming it's self explanatory, since VACUUM FULL is one of the 3 only
ways to fix a possibly corrupt index (on top of REINDEX and ALTER INDEX ...
ALTER COLLATION ... REFRESH VERSION).  I can mention it if needed though.


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

Oops, will fix.


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


Because hash indexes don't rely on the sort order for the key columns?  So even
if the sort order changes the index won't get corrupted (unless it's a non
deterministic collation of course).


> 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);
> +}


This is handled in index_create():

+       /*
+        * For deterministic transaction, only track the version if the AM
+        * relies on a stable ordering.
+        */
+       if (determ_colls)
+       {
+           bool track_version;
+
+           track_version = index_depends_stable_coll_order(indexInfo->ii_Am);
+
+           recordDependencyOnCollations(&myself, determ_colls, track_version);


> And how is that even extensible for custom index AMs?  There are other
> things than bloom out there.


That's not extensible, and that was discussed a month ago at
https://www.postgresql.org/message-id/CA%2BhUKGJ-TqYomCAYgJt53_0b9KmfSyD2qW59xfzmZa3ftRJFzA%40mail.gmail.com.

Thomas was in favor of handling that at the operator level, but it seems to me
that this would require quite a lot of extra work (assuming that we'd need to
find if the opclass handles any operator different from BTEqualStrategyNumber),
but I'm still not sure that it's sufficient to prove that an AM doesn't
internally rely on a stable ordering or not.  So I'd be in favor of adding a
new field in IndexAmRoutine for that.  Without consensus on that, I choose the
quickest approach.  Do you have any opinion on that?


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

Ok!