From 72d611109e65e279aa5719aba9831761f6f6c368 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Sat, 21 Jan 2023 13:10:29 -0500 Subject: [PATCH] Allow REPLICA IDENTITY to be set on an index that's not (yet) valid. The motivation for this change is that when pg_dump dumps a partitioned index that's marked REPLICA IDENTITY, it generates a command sequence that applies REPLICA IDENTITY before the partitioned index has been marked valid, causing restore to fail. We could perhaps change pg_dump to not do it like that, but that would be difficult and would not fix existing dump files with the problem. There seems to be very little reason for the backend to disallow this anyway --- the code ignores indisreplident when the index isn't valid --- so instead let's fix it by allowing the case. Commit 9511fb37a previously expressed a concern that allowing indisreplident to be set on invalid indexes might allow us to wind up in a situation where a table could have indisreplident set on multiple indexes. I'm not sure I follow that concern exactly, but in any case the only way that could happen is because relation_mark_replica_identity is too trusting about the existing set of markings being valid. Let's just rip out its early-exit code path (which sure looks like premature optimization anyway; what are we doing expending code to make redundant ALTER TABLE ... REPLICA IDENTITY commands marginally faster and not-redundant ones marginally slower?) and fix it to positively guarantee that no more than one index is marked indisreplident. The pg_dump failure can be demonstrated in all supported branches, so back-patch all the way. I chose to back-patch 9511fb37a as well, just to keep indisreplident handling the same in all branches. Per bug #17756 from Sergey Belyashov. Discussion: https://postgr.es/m/17756-dd50e8e0c8dd4a40@postgresql.org --- src/backend/catalog/index.c | 8 ++- src/backend/commands/tablecmds.c | 64 +++++++------------ .../regress/expected/replica_identity.out | 37 +++++++++++ src/test/regress/sql/replica_identity.sql | 20 ++++++ 4 files changed, 85 insertions(+), 44 deletions(-) diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c index 64a45f4ed4..47c84f4a07 100644 --- a/src/backend/catalog/index.c +++ b/src/backend/catalog/index.c @@ -1568,7 +1568,6 @@ index_concurrently_swap(Oid newIndexId, Oid oldIndexId, const char *oldName) /* Preserve indisreplident in the new index */ newIndexForm->indisreplident = oldIndexForm->indisreplident; - oldIndexForm->indisreplident = false; /* Preserve indisclustered in the new index */ newIndexForm->indisclustered = oldIndexForm->indisclustered; @@ -1580,6 +1579,7 @@ index_concurrently_swap(Oid newIndexId, Oid oldIndexId, const char *oldName) newIndexForm->indisvalid = true; oldIndexForm->indisvalid = false; oldIndexForm->indisclustered = false; + oldIndexForm->indisreplident = false; CatalogTupleUpdate(pg_index, &oldIndexTuple->t_self, oldIndexTuple); CatalogTupleUpdate(pg_index, &newIndexTuple->t_self, newIndexTuple); @@ -3463,10 +3463,12 @@ 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. For + * cleanliness, also clear indisreplident. */ indexForm->indisvalid = false; indexForm->indisclustered = false; + indexForm->indisreplident = false; break; case INDEX_DROP_SET_DEAD: @@ -3478,6 +3480,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 bcc7d3c29c..5d0dcfa3e6 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -14892,7 +14892,10 @@ ATExecDropOf(Relation rel, LOCKMODE lockmode) * relation_mark_replica_identity: Update a table's replica identity * * Iff ri_type = REPLICA_IDENTITY_INDEX, indexOid must be the Oid of a suitable - * index. Otherwise, it should be InvalidOid. + * index. Otherwise, it must be InvalidOid. + * + * Caller had better hold an exclusive lock on the relation, as the results + * of running two of these concurrently wouldn't be pretty. */ static void relation_mark_replica_identity(Relation rel, char ri_type, Oid indexOid, @@ -14904,7 +14907,6 @@ relation_mark_replica_identity(Relation rel, char ri_type, Oid indexOid, HeapTuple pg_index_tuple; Form_pg_class pg_class_form; Form_pg_index pg_index_form; - ListCell *index; /* @@ -14926,29 +14928,7 @@ relation_mark_replica_identity(Relation rel, char ri_type, Oid indexOid, heap_freetuple(pg_class_tuple); /* - * Check whether the correct index is marked indisreplident; if so, we're - * done. - */ - if (OidIsValid(indexOid)) - { - Assert(ri_type == REPLICA_IDENTITY_INDEX); - - pg_index_tuple = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(indexOid)); - if (!HeapTupleIsValid(pg_index_tuple)) - elog(ERROR, "cache lookup failed for index %u", indexOid); - pg_index_form = (Form_pg_index) GETSTRUCT(pg_index_tuple); - - if (pg_index_form->indisreplident) - { - ReleaseSysCache(pg_index_tuple); - return; - } - ReleaseSysCache(pg_index_tuple); - } - - /* - * Clear the indisreplident flag from any index that had it previously, - * and set it for any index that should have it now. + * Update the per-index indisreplident flags correctly. */ pg_index = table_open(IndexRelationId, RowExclusiveLock); foreach(index, RelationGetIndexList(rel)) @@ -14962,19 +14942,23 @@ relation_mark_replica_identity(Relation rel, char ri_type, Oid indexOid, elog(ERROR, "cache lookup failed for index %u", thisIndexOid); pg_index_form = (Form_pg_index) GETSTRUCT(pg_index_tuple); - /* - * Unset the bit if set. We know it's wrong because we checked this - * earlier. - */ - if (pg_index_form->indisreplident) + if (thisIndexOid == indexOid) { - dirty = true; - pg_index_form->indisreplident = false; + /* Set the bit if not already set. */ + if (!pg_index_form->indisreplident) + { + dirty = true; + pg_index_form->indisreplident = true; + } } - else if (thisIndexOid == indexOid) + else { - dirty = true; - pg_index_form->indisreplident = true; + /* Unset the bit if set. */ + if (pg_index_form->indisreplident) + { + dirty = true; + pg_index_form->indisreplident = false; + } } if (dirty) @@ -14985,7 +14969,9 @@ relation_mark_replica_identity(Relation rel, char ri_type, Oid indexOid, /* * Invalidate the relcache for the table, so that after we commit * all sessions will refresh the table's replica identity index - * before attempting any UPDATE or DELETE on the table. + * before attempting any UPDATE or DELETE on the table. (If we + * changed the table's pg_class row above, then a relcache inval + * is already queued due to that; but we might not have.) */ CacheInvalidateRelcache(rel); } @@ -15071,12 +15057,6 @@ ATExecReplicaIdentity(Relation rel, ReplicaIdentityStmt *stmt, LOCKMODE lockmode (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("cannot use partial index \"%s\" as replica identity", RelationGetRelationName(indexRel)))); - /* And neither are invalid indexes. */ - if (!indexRel->rd_index->indisvalid) - ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("cannot use invalid index \"%s\" as replica identity", - RelationGetRelationName(indexRel)))); /* Check index for nullable columns. */ for (key = 0; key < IndexRelationGetNumberOfKeyAttributes(indexRel); key++) diff --git a/src/test/regress/expected/replica_identity.out b/src/test/regress/expected/replica_identity.out index e25ec06a84..7d798ef2a5 100644 --- a/src/test/regress/expected/replica_identity.out +++ b/src/test/regress/expected/replica_identity.out @@ -227,7 +227,44 @@ Indexes: -- used as replica identity. ALTER TABLE test_replica_identity3 ALTER COLUMN id DROP NOT NULL; ERROR: column "id" is in index used as replica identity +-- +-- Test that replica identity can be set on an index that's not yet valid. +-- (This matches the way pg_dump will try to dump a partitioned table.) +-- +CREATE TABLE test_replica_identity4(id integer NOT NULL) PARTITION BY LIST (id); +CREATE TABLE test_replica_identity4_1(id integer NOT NULL); +ALTER TABLE ONLY test_replica_identity4 + ATTACH PARTITION test_replica_identity4_1 FOR VALUES IN (1); +ALTER TABLE ONLY test_replica_identity4 + ADD CONSTRAINT test_replica_identity4_pkey PRIMARY KEY (id); +ALTER TABLE ONLY test_replica_identity4 + REPLICA IDENTITY USING INDEX test_replica_identity4_pkey; +ALTER TABLE ONLY test_replica_identity4_1 + ADD CONSTRAINT test_replica_identity4_1_pkey PRIMARY KEY (id); +\d+ test_replica_identity4 + Partitioned table "public.test_replica_identity4" + Column | Type | Collation | Nullable | Default | Storage | Stats target | Description +--------+---------+-----------+----------+---------+---------+--------------+------------- + id | integer | | not null | | plain | | +Partition key: LIST (id) +Indexes: + "test_replica_identity4_pkey" PRIMARY KEY, btree (id) INVALID REPLICA IDENTITY +Partitions: test_replica_identity4_1 FOR VALUES IN (1) + +ALTER INDEX test_replica_identity4_pkey + ATTACH PARTITION test_replica_identity4_1_pkey; +\d+ test_replica_identity4 + Partitioned table "public.test_replica_identity4" + Column | Type | Collation | Nullable | Default | Storage | Stats target | Description +--------+---------+-----------+----------+---------+---------+--------------+------------- + id | integer | | not null | | plain | | +Partition key: LIST (id) +Indexes: + "test_replica_identity4_pkey" PRIMARY KEY, btree (id) REPLICA IDENTITY +Partitions: test_replica_identity4_1 FOR VALUES IN (1) + DROP TABLE test_replica_identity; DROP TABLE test_replica_identity2; DROP TABLE test_replica_identity3; +DROP TABLE test_replica_identity4; DROP TABLE test_replica_identity_othertable; diff --git a/src/test/regress/sql/replica_identity.sql b/src/test/regress/sql/replica_identity.sql index 33da829713..14620b7713 100644 --- a/src/test/regress/sql/replica_identity.sql +++ b/src/test/regress/sql/replica_identity.sql @@ -98,7 +98,27 @@ ALTER TABLE test_replica_identity3 ALTER COLUMN id TYPE bigint; -- used as replica identity. ALTER TABLE test_replica_identity3 ALTER COLUMN id DROP NOT NULL; +-- +-- Test that replica identity can be set on an index that's not yet valid. +-- (This matches the way pg_dump will try to dump a partitioned table.) +-- +CREATE TABLE test_replica_identity4(id integer NOT NULL) PARTITION BY LIST (id); +CREATE TABLE test_replica_identity4_1(id integer NOT NULL); +ALTER TABLE ONLY test_replica_identity4 + ATTACH PARTITION test_replica_identity4_1 FOR VALUES IN (1); +ALTER TABLE ONLY test_replica_identity4 + ADD CONSTRAINT test_replica_identity4_pkey PRIMARY KEY (id); +ALTER TABLE ONLY test_replica_identity4 + REPLICA IDENTITY USING INDEX test_replica_identity4_pkey; +ALTER TABLE ONLY test_replica_identity4_1 + ADD CONSTRAINT test_replica_identity4_1_pkey PRIMARY KEY (id); +\d+ test_replica_identity4 +ALTER INDEX test_replica_identity4_pkey + ATTACH PARTITION test_replica_identity4_1_pkey; +\d+ test_replica_identity4 + DROP TABLE test_replica_identity; DROP TABLE test_replica_identity2; DROP TABLE test_replica_identity3; +DROP TABLE test_replica_identity4; DROP TABLE test_replica_identity_othertable;