Re: Collation versioning

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

If you add a call to index_update_collation_versions(), the old and
invalid index will use the same refobjversion as the new index, which
is the latest collation version of the system, no?  If the operation
is interrupted before the invalid index is dropped, then we would keep
a confusing value for refobjversion, because the old invalid index
does not rely on the new collation version, but on the old one.
Hence, it seems to me that it would be correct to have the old invalid
index either use an empty version string to say "we don't know"
because the index is invalid anyway, or keep a reference to the old
collation version intact.  I think that the latter is much more useful
for debugging issues when upgrading a subset of indexes if the
operation is interrupted for a reason or another.

> 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:
> 
> =# UPDATE pg_depend SET refobjversion = 'meh' WHERE refobjversion = '153.97';
> UPDATE 1

Updates to catalogs are not an existing practice in the core
regression tests, so patches had better not do that. :p 

> =# 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

I guess that you used a second session here beginning a transaction
before REINDEX CONCURRENTLY ran here so as it would stop after
swapping dependencies, right?

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

After a non-concurrent reindex, this information is right.  However
based on the output of your test here, after REINDEX CONCURRENTLY the
information held in refobjversion is incorrect for t1_val_idx_ccold
and t1_val_idx.  They should be reversed.
--
Michael