On Tue, Aug 25, 2020 at 08:59:37PM +0530, Rahila wrote: > Sorry, I didn't apply correctly. The tests pass for me. In addition, I > tested with partitioned tables. It works as expected and makes the REPLICA > IDENTITY 'n' for the partitions as well when an index on a partitioned > table is dropped.
Indeed. I have added a test for that. While looking at this patch again, I noticed that the new tests for contrib/test_decoding/ and the improvements for relreplident are really two different bullet points, and that the new tests should not be impacted as long as we switch to NOTHING the replica identity once its index is dropped. So, for now, I have applied the new decoding tests with a first commit, and attached is an updated patch which includes tests in the main regression test suite for replica_identity.sql, which is more consistent with the rest as that's the main place where we look at the state of pg_class.relreplident. I'll go through this part again tomorrow, it is late here. -- Michael
From 277d9b53e2be9098e72167b5de2a52c40e6e8542 Mon Sep 17 00:00:00 2001 From: Michael Paquier <mich...@paquier.xyz> Date: Wed, 26 Aug 2020 20:59:34 +0900 Subject: [PATCH v3] 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 | 12 ++++ 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, 104 insertions(+), 23 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..d344a070ce 100644 --- a/src/backend/catalog/index.c +++ b/src/backend/catalog/index.c @@ -2195,6 +2195,18 @@ index_drop(Oid indexId, bool concurrent, bool concurrent_lock_mode) if (userIndexRelation->rd_rel->relkind != RELKIND_PARTITIONED_INDEX) RelationDropStorage(userIndexRelation); + /* + * If the index to be dropped is marked as indisreplident, reset + * pg_class.relreplident for the parent table. + */ + if (userIndexRelation->rd_index->indisreplident) + { + SetRelationReplIdent(heapId, REPLICA_IDENTITY_NOTHING); + + /* make the update visible */ + CommandCounterIncrement(); + } + /* * Close and flush the index's relcache entry, to ensure relcache doesn't * try to rebuild it while we're deleting catalog entries. We keep the 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..340f8615f5 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 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..18220a595b 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 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
