On Fri, Mar 20, 2020 at 02:52:33PM +0100, Julien Rouhaud wrote:
> On Thu, Mar 19, 2020 at 08:12:47PM +0100, Julien Rouhaud wrote:
> > On Thu, Mar 19, 2020 at 12:31:54PM +0900, Michael Paquier wrote:
> > > 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.
> >
> > Indeed, I confused the _ccold and _ccnew indexes. So, the root cause is phase
> > 4, more precisely the dependency swap in index_concurrently_swap.
> >
> > A possible fix would be to teach changeDependenciesOf() to preserve the
> > dependency version. It'd be quite bit costly as this would mean an extra index
> > search for each dependency row found. We could probably skip the lookup if the
> > row have a NULL recorded version, as version should either be null or non null
> > for both objects.
> >
> > I'm wondering if that's a good time to make changeDependenciesOf and
> > changeDependenciesOn private, and instead expose a swapDependencies(classid,
> > obj1, obj2) that would call both, as preserving the version doesn't really
> > makes sense outside a switch. It's als oa good way to ensure that no CCI is
> > performed in the middle.
>
> Hearing no complaints, I implemented that approach in attached v18.
>
> Here's the new behavior for interrupted REINDEX CONCURRENTLY:
>
> # drop table if exists t1;create table t1(id integer, val text); create index on t1(val collate "fr-x-icu");
> NOTICE: 00000: table "t1" does not exist, skipping
> DROP TABLE
> CREATE TABLE
> CREATE INDEX
>
> # update pg_depend set refobjversion = 'meh' where refobjversion = '153.97';
> UPDATE 1
>
> # 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 | t | meh
> (1 row)
>
> (on another session: begin; select * from t1 for update;)
>
> # reindex table CONCURRENTLY t1;
> ^CCancel request sent
> ERROR: 57014: canceling statement due to user request
>
> # 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 | meh
> t1_val_idx | t | 153.97
> (2 rows)
>
> # reindex table CONCURRENTLY t1;
> WARNING: 0A000: cannot reindex invalid index "public.t1_val_idx_ccold" concurrently, skipping
> WARNING: XX002: cannot reindex invalid index "pg_toast.pg_toast_16385_index_ccold" concurrently, skipping
> 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 | f | meh
> t1_val_idx | t | 153.97
> (2 rows)
>
> # reindex table t1;
> WARNING: 0A000: cannot reindex invalid index "pg_toast.pg_toast_16385_index_ccold" on TOAST table, skipping
> 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)
>
>
> I also rebased the patchset against master (so removing the regcollation
> patch), but no other changes otherwise, so there's still the direct updates on
> the catalog in the regressoin tests.
Conflict since 2f9eb3132 (pg_dump: Allow dumping data of specific foreign
servers), v19 attached.