Re: Collation versioning

On Wed, Mar 18, 2020 at 09:56:40AM +0100, Julien Rouhaud wrote:
> On Wed, Mar 18, 2020 at 04:55:25PM +0900, Michael Paquier 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.

I run a pgindent on all .[ch] files and kept only the relevant changes, for
each patch, so this should now be fine.


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


I changed 0002 to have the version as the 4th argument just in case.


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


AFAICT it was only missing a call to index_update_collation_versions() in
ReindexRelationConcurrently.  I added regression tests to make sure that
REINDEX, REINDEX [INDEX|TABLE] CONCURRENTLY and VACUUM FULL are doing what's
expected.

Given discussion in nearby threads, I obviously can't add tests for failed
REINDEX CONCURRENTLY, so here's what's happening with a manual repro:

=# CREATE TABLE t1(id integer, val text);
CREATE

=# CREATE INDEX ON t1(val COLLATE "fr-x-icu");
CREATE

=# UPDATE pg_depend SET refobjversion = 'meh' WHERE refobjversion = '153.97';
UPDATE 1

=# REINDEX TABLE CONCURRENTLY t1 ;
LOCATION:  ReindexRelationConcurrently, indexcmds.c:2839
^CCancel request sent
ERROR:  57014: canceling statement due to user request
LOCATION:  ProcessInterrupts, postgres.c:3171

=# SELECT objid::regclass, indisvalid, refobjversion
   FROM pg_depend d
   JOIN pg_index i ON i.indexrelid = d.objid
   WHERE refobjversion IS NOT NULL;
      objid       | indisvalid | refobjversion
------------------+------------+---------------
 t1_val_idx_ccold | f          | 153.97
 t1_val_idx       | t          | meh
(2 rows)

=# REINDEX TABLE t1;
WARNING:  0A000: cannot reindex invalid index "pg_toast.pg_toast_16418_index_ccold" on TOAST table, skipping
LOCATION:  reindex_relation, index.c:3987
REINDEX

=# SELECT objid::regclass, indisvalid, refobjversion
   FROM pg_depend d
   JOIN pg_index i ON i.indexrelid = d.objid
   WHERE refobjversion IS NOT NULL;
      objid       | indisvalid | refobjversion
------------------+------------+---------------
 t1_val_idx_ccold | t          | 153.97
 t1_val_idx       | t          | 153.97
(2 rows)

ISTM that it's working as intended.

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


No change here.


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


Explanation added.

v17 attached, rebased against master (conflict since 8408e3a557).