On Wed, Aug 26, 2020 at 09:08:51PM +0900, Michael Paquier wrote: > I'll go through this part again tomorrow, it is late here.
There is actually a huge gotcha here that exists since replica identities exist: relation_mark_replica_identity() only considers valid indexes! So, on HEAD, if DROP INDEX CONCURRENTLY fails in the second transaction doing the physical drop, then we would finish with a catalog entry that has indisreplident and !indisinvalid. If you reindex it afterwards and switch the index back to be valid, there can be more than one valid index marked with indisreplident, which messes up with the logic in tablecmds.c because we use RelationGetIndexList(), that discards invalid indexes. This case is actually rather similar to the handling of indisclustered. So we have a set of two issues here: 1) indisreplident should be switched to false when we clear the valid flag of an index for INDEX_DROP_CLEAR_VALID. This issue exists since 9.4. 2) To never finish in a state where we have REPLICA IDENTITY INDEX without an index marked as indisreplident, we need to reset the replident of the parent table in the same transaction as the one clearing indisvalid for the concurrent case. That's a problem of the patch proposed upthread. Attached is a patch for 1) and 2) grouped together, to ease review for now. I think that we had better fix 1) separately though, so I am going to start a new thread about that with a separate patch as the current thread is misleading. -- Michael
From 8893acae8b4d14b1cccb3bdcf8c23e46215d2060 Mon Sep 17 00:00:00 2001 From: Michael Paquier <mich...@paquier.xyz> Date: Thu, 27 Aug 2020 11:27:04 +0900 Subject: [PATCH v4] Reset pg_class.relreplident when associated replica index is dropped When an index associated to REPLICA IDENTITY USING INDEX gets dropped, relreplident gets set to 'i', which can be confusing as the catalogs have a state inconsistent with pg_index that lacks an associated entry marked with indisreplident. This changes the logic so as such an index dropped leads to 'n' to be set for the parent table, equivalent to REPLICA IDENTITY NOTHING. Author: Michael Paquier Reviewed-by: Masahiko Sawada, Rahila Syed, Euler Taveira Discussion: https://postgr.es/m/20200522035028.go2...@paquier.xyz --- src/include/commands/tablecmds.h | 2 + src/backend/catalog/index.c | 43 +++++++++++++- src/backend/commands/tablecmds.c | 58 ++++++++++++------- .../regress/expected/replica_identity.out | 30 ++++++++++ src/test/regress/sql/replica_identity.sql | 22 +++++++ doc/src/sgml/catalogs.sgml | 3 +- 6 files changed, 132 insertions(+), 26 deletions(-) diff --git a/src/include/commands/tablecmds.h b/src/include/commands/tablecmds.h index c1581ad178..b163a82c52 100644 --- a/src/include/commands/tablecmds.h +++ b/src/include/commands/tablecmds.h @@ -61,6 +61,8 @@ extern void ExecuteTruncateGuts(List *explicit_rels, List *relids, List *relids_ extern void SetRelationHasSubclass(Oid relationId, bool relhassubclass); +extern void SetRelationReplIdent(Oid relationId, char ri_type); + extern ObjectAddress renameatt(RenameStmt *stmt); extern ObjectAddress RenameConstraint(RenameStmt *stmt); diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c index 1be27eec52..ccdda7561a 100644 --- a/src/backend/catalog/index.c +++ b/src/backend/catalog/index.c @@ -2005,6 +2005,7 @@ index_drop(Oid indexId, bool concurrent, bool concurrent_lock_mode) Relation indexRelation; HeapTuple tuple; bool hasexprs; + bool resetreplident; LockRelId heaprelid, indexrelid; LOCKTAG heaplocktag; @@ -2048,6 +2049,15 @@ index_drop(Oid indexId, bool concurrent, bool concurrent_lock_mode) */ CheckTableNotInUse(userIndexRelation, "DROP INDEX"); + /* + * Check if the REPLICA IDENTITY of the parent relation needs to be + * reset. This should be done only if the index to drop here is + * marked as used in a replica identity. We cannot rely on the + * contents of pg_index for the index either as a concurrent drop + * may have changed indisreplident already. + */ + resetreplident = userIndexRelation->rd_index->indisreplident; + /* * Drop Index Concurrently is more or less the reverse process of Create * Index Concurrently. @@ -2159,10 +2169,32 @@ index_drop(Oid indexId, bool concurrent, bool concurrent_lock_mode) /* Finish invalidation of index and mark it as dead */ index_concurrently_set_dead(heapId, indexId); + } + /* + * If the index to be dropped was marked as indisreplident (it could + * have been updated when clearing indisvalid previously), reset + * pg_class.relreplident for the parent table. Note that this part + * needs to be done in the same transaction as the one marking the + * index as invalid, so as we never finish with the case where the + * parent's replica identity is based on an index, without any index + * marked with indisreplident. + */ + if (resetreplident) + { + SetRelationReplIdent(heapId, REPLICA_IDENTITY_NOTHING); + + /* make the update visible, mandatory for the non-concurrent case */ + CommandCounterIncrement(); + } + + /* continue the concurrent process */ + if (concurrent) + { /* - * Again, commit the transaction to make the pg_index update visible - * to other sessions. + * Again, commit the transaction to make the pg_index and pg_class + * (in the case where indisreplident is set) updates are visible to + * other sessions. */ CommitTransactionCommand(); StartTransactionCommand(); @@ -3349,10 +3381,13 @@ index_set_state_flags(Oid indexId, IndexStateFlagsAction action) * CONCURRENTLY that failed partway through.) * * Note: the CLUSTER logic assumes that indisclustered cannot be - * set on any invalid index, so clear that flag too. + * set on any invalid index, so clear that flag too. Similarly, + * ALTER TABLE assumes that indisreplident cannot be set for + * invalid indexes. */ indexForm->indisvalid = false; indexForm->indisclustered = false; + indexForm->indisreplident = false; break; case INDEX_DROP_SET_DEAD: @@ -3364,6 +3399,8 @@ index_set_state_flags(Oid indexId, IndexStateFlagsAction action) * the index at all. */ Assert(!indexForm->indisvalid); + Assert(!indexForm->indisclustered); + Assert(!indexForm->indisreplident); indexForm->indisready = false; indexForm->indislive = false; break; diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index d2b15a3387..968cb41f98 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -3020,6 +3020,41 @@ SetRelationHasSubclass(Oid relationId, bool relhassubclass) table_close(relationRelation, RowExclusiveLock); } +/* + * SetRelationReplIdent + * Set the value of the relation's relreplident field in pg_class. + * + * NOTE: caller must be holding an appropriate lock on the relation. + * ShareUpdateExclusiveLock is sufficient. + */ +void +SetRelationReplIdent(Oid relationId, char ri_type) +{ + Relation relationRelation; + HeapTuple tuple; + Form_pg_class classtuple; + + /* + * Fetch a modifiable copy of the tuple, modify it, update pg_class. + */ + relationRelation = table_open(RelationRelationId, RowExclusiveLock); + tuple = SearchSysCacheCopy1(RELOID, + ObjectIdGetDatum(relationId)); + if (!HeapTupleIsValid(tuple)) + elog(ERROR, "cache lookup failed for relation \"%s\"", + get_rel_name(relationId)); + classtuple = (Form_pg_class) GETSTRUCT(tuple); + + if (classtuple->relreplident != ri_type) + { + classtuple->relreplident = ri_type; + CatalogTupleUpdate(relationRelation, &tuple->t_self, tuple); + } + + heap_freetuple(tuple); + table_close(relationRelation, RowExclusiveLock); +} + /* * renameatt_check - basic sanity checks before attribute rename */ @@ -14487,31 +14522,12 @@ relation_mark_replica_identity(Relation rel, char ri_type, Oid indexOid, bool is_internal) { Relation pg_index; - Relation pg_class; - HeapTuple pg_class_tuple; HeapTuple pg_index_tuple; - Form_pg_class pg_class_form; Form_pg_index pg_index_form; - ListCell *index; - /* - * Check whether relreplident has changed, and update it if so. - */ - pg_class = table_open(RelationRelationId, RowExclusiveLock); - pg_class_tuple = SearchSysCacheCopy1(RELOID, - ObjectIdGetDatum(RelationGetRelid(rel))); - if (!HeapTupleIsValid(pg_class_tuple)) - elog(ERROR, "cache lookup failed for relation \"%s\"", - RelationGetRelationName(rel)); - pg_class_form = (Form_pg_class) GETSTRUCT(pg_class_tuple); - if (pg_class_form->relreplident != ri_type) - { - pg_class_form->relreplident = ri_type; - CatalogTupleUpdate(pg_class, &pg_class_tuple->t_self, pg_class_tuple); - } - table_close(pg_class, RowExclusiveLock); - heap_freetuple(pg_class_tuple); + /* update relreplident if necessary */ + SetRelationReplIdent(RelationGetRelid(rel), ri_type); /* * Check whether the correct index is marked indisreplident; if so, we're diff --git a/src/test/regress/expected/replica_identity.out b/src/test/regress/expected/replica_identity.out index 79002197a7..86053c5bb4 100644 --- a/src/test/regress/expected/replica_identity.out +++ b/src/test/regress/expected/replica_identity.out @@ -227,3 +227,33 @@ DROP TABLE test_replica_identity; DROP TABLE test_replica_identity2; DROP TABLE test_replica_identity3; DROP TABLE test_replica_identity_othertable; +-- +-- Test that DROP INDEX on a index used for REPLICA IDENTITY sets +-- relreplident back to NOTHING. +-- +CREATE TABLE test_replica_identity_4 (id int NOT NULL); +CREATE UNIQUE INDEX test_replica_index_4 ON test_replica_identity_4(id); +ALTER TABLE test_replica_identity_4 REPLICA IDENTITY + USING INDEX test_replica_index_4; +DROP INDEX CONCURRENTLY test_replica_index_4; +SELECT relreplident FROM pg_class WHERE oid = 'test_replica_identity_4'::regclass; + relreplident +-------------- + n +(1 row) + +DROP TABLE test_replica_identity_4; +-- Partitioned table +CREATE TABLE test_replica_part (id int NOT NULL) + PARTITION BY RANGE (id); +CREATE UNIQUE INDEX test_replica_part_index ON test_replica_part(id); +ALTER TABLE test_replica_part REPLICA IDENTITY + USING INDEX test_replica_part_index; +DROP INDEX test_replica_part_index; +SELECT relreplident FROM pg_class WHERE oid = 'test_replica_part'::regclass; + relreplident +-------------- + n +(1 row) + +DROP TABLE test_replica_part; diff --git a/src/test/regress/sql/replica_identity.sql b/src/test/regress/sql/replica_identity.sql index a5ac8f5567..6dd34dc39d 100644 --- a/src/test/regress/sql/replica_identity.sql +++ b/src/test/regress/sql/replica_identity.sql @@ -98,3 +98,25 @@ DROP TABLE test_replica_identity; DROP TABLE test_replica_identity2; DROP TABLE test_replica_identity3; DROP TABLE test_replica_identity_othertable; + +-- +-- Test that DROP INDEX on a index used for REPLICA IDENTITY sets +-- relreplident back to NOTHING. +-- + +CREATE TABLE test_replica_identity_4 (id int NOT NULL); +CREATE UNIQUE INDEX test_replica_index_4 ON test_replica_identity_4(id); +ALTER TABLE test_replica_identity_4 REPLICA IDENTITY + USING INDEX test_replica_index_4; +DROP INDEX CONCURRENTLY test_replica_index_4; +SELECT relreplident FROM pg_class WHERE oid = 'test_replica_identity_4'::regclass; +DROP TABLE test_replica_identity_4; +-- Partitioned table +CREATE TABLE test_replica_part (id int NOT NULL) + PARTITION BY RANGE (id); +CREATE UNIQUE INDEX test_replica_part_index ON test_replica_part(id); +ALTER TABLE test_replica_part REPLICA IDENTITY + USING INDEX test_replica_part_index; +DROP INDEX test_replica_part_index; +SELECT relreplident FROM pg_class WHERE oid = 'test_replica_part'::regclass; +DROP TABLE test_replica_part; diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml index 9fe260ecff..601b3d448c 100644 --- a/doc/src/sgml/catalogs.sgml +++ b/doc/src/sgml/catalogs.sgml @@ -2144,8 +2144,7 @@ SCRAM-SHA-256$<replaceable><iteration count></replaceable>:<replaceable>&l <literal>n</literal> = nothing, <literal>f</literal> = all columns, <literal>i</literal> = index with - <structfield>indisreplident</structfield> set (same as nothing if the - index used has been dropped) + <structfield>indisreplident</structfield> set </para></entry> </row> -- 2.28.0
signature.asc
Description: PGP signature
