Re: Collation versioning

From: Julien Rouhaud <rjuju123(at)gmail(dot)com>
To: Michael Paquier <michael(at)paquier(dot)xyz>
Cc: Thomas Munro <thomas(dot)munro(at)gmail(dot)com>, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, Laurenz Albe <laurenz(dot)albe(at)cybertec(dot)at>, Douglas Doole <dougdoole(at)gmail(dot)com>, Christoph Berg <myon(at)debian(dot)org>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Collation versioning
Date: 2020-03-20 13:52:33
Message-ID: 20200320135233.GA52612@nol
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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.

Attachment Content-Type Size
v18-0001-Remove-pg_collation.collversion.patch text/plain 26.8 KB
v18-0002-Add-pg_depend.refobjversion.patch text/plain 12.3 KB
v18-0003-Track-collation-versions-for-indexes.patch text/plain 69.8 KB
v18-0004-Preserve-index-dependencies-on-collation-during-.patch text/plain 38.6 KB
v18-0005-Add-ALTER-INDEX-.-ALTER-COLLATION-.-REFRESH-VERS.patch text/plain 14.6 KB
v18-0006-doc-Add-Collation-Versions-section.patch text/plain 2.5 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Sergei Kornilov 2020-03-20 14:09:05 Re: Planning counters in pg_stat_statements (using pgss_store)
Previous Message Laurenz Albe 2020-03-20 13:43:20 Re: Berserk Autovacuum (let's save next Mandrill)