On Thu, Aug 27, 2020 at 11:28:35AM +0900, Michael Paquier wrote: > 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.
A fix for consistency problem with indisreplident and invalid indexes has been committed as of 9511fb37. Attached is a rebased patch, where I noticed two incorrect things: - The comment of REPLICA_IDENTITY_INDEX is originally incorrect. If the index is dropped, the replica index switches back to nothing. - relreplident was getting reset one transaction too late, when the old index is marked as dead. -- Michael
From fa7585609a614bdba27bc2e3199379d04fc0eabd Mon Sep 17 00:00:00 2001 From: Michael Paquier <mich...@paquier.xyz> Date: Sun, 30 Aug 2020 16:02:32 +0900 Subject: [PATCH v5] 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/catalog/pg_class.h | 6 +- src/include/commands/tablecmds.h | 2 + src/backend/catalog/index.c | 37 +++++++++++- 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 +- 7 files changed, 128 insertions(+), 30 deletions(-) diff --git a/src/include/catalog/pg_class.h b/src/include/catalog/pg_class.h index 78b33b2a7f..4f9ce17651 100644 --- a/src/include/catalog/pg_class.h +++ b/src/include/catalog/pg_class.h @@ -175,11 +175,7 @@ typedef FormData_pg_class *Form_pg_class; #define REPLICA_IDENTITY_NOTHING 'n' /* all columns are logged as replica identity */ #define REPLICA_IDENTITY_FULL 'f' -/* - * an explicitly chosen candidate key's columns are used as replica identity. - * Note this will still be set if the index has been dropped; in that case it - * has the same meaning as 'd'. - */ +/* an explicitly-chosen candidate key's columns are used as replica identity */ #define REPLICA_IDENTITY_INDEX 'i' /* 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 62e487bb4c..61d05495b4 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 as a concurrent drop would have + * reset indisreplident already, so save the existing value first. + */ + resetreplident = userIndexRelation->rd_index->indisreplident; + /* * Drop Index Concurrently is more or less the reverse process of Create * Index Concurrently. @@ -2132,7 +2142,29 @@ index_drop(Oid indexId, bool concurrent, bool concurrent_lock_mode) */ LockRelationIdForSession(&heaprelid, ShareUpdateExclusiveLock); LockRelationIdForSession(&indexrelid, ShareUpdateExclusiveLock); + } + /* + * If the index to be dropped was marked as indisreplident (note that it + * would have been reset 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 relation uses REPLICA_IDENTITY_INDEX, without any index marked + * with indisreplident. + */ + if (resetreplident) + { + SetRelationReplIdent(heapId, REPLICA_IDENTITY_NOTHING); + + /* make the update visible, only for the non-concurrent case */ + if (!concurrent) + CommandCounterIncrement(); + } + + /* continue the concurrent process */ + if (concurrent) + { PopActiveSnapshot(); CommitTransactionCommand(); StartTransactionCommand(); @@ -2161,8 +2193,9 @@ index_drop(Oid indexId, bool concurrent, bool concurrent_lock_mode) index_concurrently_set_dead(heapId, indexId); /* - * 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(); 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
