8000 Allow REPLICA IDENTITY to be set on an index that's not (yet) valid. · postgres/postgres@6c122ed · GitHub
[go: up one dir, main page]
Skip to content

Commit 6c122ed

Browse files
tglsfdctglsfdc
committed
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 9511fb3 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 9511fb3 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
1 parent 8f70de7 commit 6c122ed

File tree

4 files changed

+84
-43
lines changed

4 files changed

+84
-43
lines changed

‎src/backend/catalog/index.c

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3692,10 +3692,12 @@ index_set_state_flags(Oid indexId, IndexStateFlagsAction action)
36923692
* CONCURRENTLY that failed partway through.)
36933693
*
36943694
* Note: the CLUSTER logic assumes that indisclustered cannot be
3695-
* set on any invalid index, so clear that flag too.
3695+
* set on any invalid index, so clear that flag too. For
3696+
* cleanliness, also clear indisreplident.
36963697
*/
36973698
indexForm->indisvalid = false;
36983699
indexForm->indisclustered = false;
3700+
indexForm->indisreplident = false;
36993701
break;
37003702
case INDEX_DROP_SET_DEAD:
37013703

@@ -3707,6 +3709,8 @@ index_set_state_flags(Oid indexId, IndexStateFlagsAction action)
37073709
* the index at all.
37083710
*/
37093711
Assert(!indexForm->indisvalid);
3712+
Assert(!indexForm->indisclustered);
3713+
Assert(!indexForm->indisreplident);
37103714
indexForm->indisready = false;
37113715
indexForm->indislive = false;
37123716
break;

‎src/backend/commands/tablecmds.c

Lines changed: 22 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -13370,7 +13370,10 @@ ATExecDropOf(Relation rel, LOCKMODE lockmode)
1337013370
* relation_mark_replica_identity: Update a table's replica identity
1337113371
*
1337213372
* Iff ri_type = REPLICA_IDENTITY_INDEX, indexOid must be the Oid of a suitable
13373-
* index. Otherwise, it should be InvalidOid.
13373+
* index. Otherwise, it must be InvalidOid.
13374+
*
13375+
* Caller had better hold an exclusive lock on the relation, as the results
13376+
* of running two of these concurrently wouldn't be pretty.
1337413377
*/
1337513378
static void
1337613379
relation_mark_replica_identity(Relation rel, char ri_type, Oid indexOid,
@@ -13382,7 +13385,6 @@ relation_mark_replica_identity(Relation rel, char ri_type, Oid indexOid,
1338213385
HeapTuple pg_index_tuple;
1338313386
Form_pg_class pg_class_form;
1338413387
Form_pg_index pg_index_form;
13385-
1338613388
ListCell *index;
1338713389

1338813390
/*
@@ -13404,29 +13406,7 @@ relation_mark_replica_identity(Relation rel, char ri_type, Oid indexOid,
1340413406
heap_freetuple(pg_class_tuple);
1340513407

1340613408
/*
13407-
* Check whether the correct index is marked indisreplident; if so, we're
13408-
* done.
13409-
*/
13410-
if (OidIsValid(indexOid))
13411-
{
13412-
Assert(ri_type == REPLICA_IDENTITY_INDEX);
13413-
13414-
pg_index_tuple = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(indexOid));
13415-
if (!HeapTupleIsValid(pg_index_tuple))
13416-
elog(ERROR, "cache lookup failed for index %u", indexOid);
13417-
pg_index_form = (Form_pg_index) GETSTRUCT(pg_index_tuple);
13418-
13419-
if (pg_index_form->indisreplident)
13420-
{
13421-
ReleaseSysCache(pg_index_tuple);
13422-
return;
13423-
}
13424-
ReleaseSysCache(pg_index_tuple);
13425-
}
13426-
13427-
/*
13428-
* Clear the indisreplident flag from any index that had it previously,
13429-
* and set it for any index that should have it now.
13409+
* Update the per-index indisreplident flags correctly.
1343013410
*/
1343113411
pg_index = heap_open(IndexRelationId, RowExclusiveLock);
1343213412
foreach(index, RelationGetIndexList(rel))
@@ -13440,19 +13420,23 @@ relation_mark_replica_identity(Relation rel, char ri_type, Oid indexOid,
1344013420
elog(ERROR, "cache lookup failed for index %u", thisIndexOid);
1344113421
pg_index_form = (Form_pg_index) GETSTRUCT(pg_index_tuple);
1344213422

13443-
/*
13444-
* Unset the bit if set. We know it's wrong because we checked this
13445-
* earlier.
13446-
*/
13447-
if (pg_index_form->indisreplident)
13423+
if (thisIndexOid == indexOid)
1344813424
{
13449-
dirty = true;
13450-
pg_index_form->indisreplident = false;
13425+
/* Set the bit if not already set. */
13426+
if (!pg_index_form->indisreplident)
13427+
{
13428+
dirty = true;
13429+
pg_index_form->indisreplident = true;
13430+
}
1345113431
}
13452-
else if (thisIndexOid == indexOid)
13432+
else
1345313433
{
13454-
dirty = true;
13455-
pg_index_form->indisreplident = true;
13434+
/* Unset the bit if set. */
13435+
if (pg_index_form->indisreplident)
13436+
{
13437+
dirty = true;
13438+
pg_index_form->indisreplident = false;
13439+
}
1345613440
}
1345713441

1345813442
if (dirty)
@@ -13463,7 +13447,9 @@ relation_mark_replica_identity(Relation rel, char ri_type, Oid indexOid,
1346313447
/*
1346413448
* Invalidate the relcache for the table, so that after we commit
1346513449
* all sessions will refresh the table's replica identity index
13466-
* before attempting any UPDATE or DELETE on the table.
13450+
* before attempting any UPDATE or DELETE on the table. (If we
13451+
* changed the table's pg_class row above, then a relcache inval
13452+
* is already queued due to that; but we might not have.)
1346713453
*/
1346813454
CacheInvalidateRelcache(rel);
1346913455
}
@@ -13549,12 +13535,6 @@ ATExecReplicaIdentity(Relation rel, ReplicaIdentityStmt *stmt, LOCKMODE lockmode
1354913535
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
1355013536
errmsg("cannot use partial index \"%s\" as replica identity",
1355113537
RelationGetRelationName(indexRel))));
13552-
/* And neither are invalid indexes. */
13553-
if (!IndexIsValid(indexRel->rd_index))
13554-
ereport(ERROR,
13555-
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
13556-
errmsg("cannot use invalid index \"%s\" as replica identity",
13557-
RelationGetRelationName(indexRel))));
1355813538

1355913539
/* Check index for nullable columns. */
1356013540
for (key = 0; key < IndexRelationGetNumberOfKeyAttributes(indexRel); key++)

‎src/test/regress/expected/replica_identity.out

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -234,7 +234,44 @@ Indexes:
234234
-- used as replica identity.
235235
ALTER TABLE test_replica_identity3 ALTER COLUMN id DROP NOT NULL;
236236
ERROR: column "id" is in index used as replica identity
237+
--
238+
-- Test that replica identity can be set on an index that's not yet valid.
239+
-- (This matches the way pg_dump will try to dump a partitioned table.)
240+
--
241+
CREATE TABLE test_replica_identity4(id integer NOT NULL) PARTITION BY LIST (id);
242+
CREATE TABLE test_replica_identity4_1(id integer NOT NULL);
243+
ALTER TABLE ONLY test_replica_identity4
244+
ATTACH PARTITION test_replica_identity4_1 FOR VALUES IN (1);
245+
ALTER TABLE ONLY test_replica_identity4
246+
ADD CONSTRAINT test_replica_identity4_pkey PRIMARY KEY (id);
247+
ALTER TABLE ONLY test_replica_identity4
248+
REPLICA IDENTITY USING INDEX test_replica_identity4_pkey;
249+
ALTER TABLE ONLY test_replica_identity4_1
250+
ADD CONSTRAINT test_replica_identity4_1_pkey PRIMARY KEY (id);
251+
\d+ test_replica_identity4
252+
Table "public.test_replica_identity4"
253+
Column | Type | Collation | Nullable | Default | Storage | Stats target | Description
254+
--------+---------+-----------+----------+---------+---------+--------------+-------------
255+
id | integer | | not null | | plain | |
256+
Partition key: LIST (id)
257+
Indexes:
258+
"test_replica_identity4_pkey" PRIMARY KEY, btree (id) INVALID REPLICA IDENTITY
259+
Partitions: test_replica_identity4_1 FOR VALUES IN (1)
260+
261+
ALTER INDEX test_replica_identity4_pkey
262+
ATTACH PARTITION test_replica_identity4_1_pkey;
263+
\d+ test_replica_identity4
264+
Table "public.test_replica_identity4"
265+
Column | Type | Collation | Nullable | Default | Storage | Stats target | Description
266+
--------+---------+-----------+----------+---------+---------+--------------+-------------
267+
id | integer | | not null | | plain | |
268+
Partition key: LIST (id)
269+
Indexes:
270+
"test_replica_identity4_pkey" PRIMARY KEY, btree (id) REPLICA IDENTITY
271+
Partitions: test_replica_identity4_1 FOR VALUES IN (1)
272+
237273
DROP TABLE test_replica_identity;
238274
DROP TABLE test_replica_identity2;
239275
DROP TABLE test_replica_identity3;
276+
DROP TABLE test_replica_identity4;
240277
DROP TABLE test_replica_identity_othertable;

‎src/test/regress/sql/replica_identity.sql

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,27 @@ ALTER TABLE test_replica_identity3 ALTER COLUMN id TYPE bigint;
102102
-- used as replica identity.
103103
ALTER TABLE test_replica_identity3 ALTER COLUMN id DROP NOT NULL;
104104

105+
--
106+
-- Test that replica identity can be set on an index that's not yet valid.
107+
-- (This matches the way pg_dump will try to dump a partitioned table.)
108+
--
109+
CREATE TABLE test_replica_identity4(id integer NOT NULL) PARTITION BY LIST (id);
110+
CREATE TABLE test_replica_identity4_1(id integer NOT NULL);
111+
ALTER TABLE ONLY test_replica_identity4
112+
ATTACH PARTITION test_replica_identity4_1 FOR VALUES IN (1);
113+
ALTER TABLE ONLY test_replica_identity4
114+
ADD CONSTRAINT test_replica_identity4_pkey PRIMARY KEY (id);
115+
ALTER TABLE ONLY test_replica_identity4
116+
REPLICA IDENTITY USING INDEX test_replica_identity4_pkey;
117+
ALTER TABLE ONLY test_replica_identity4_1
118+
ADD CONSTRAINT test_replica_identity4_1_pkey PRIMARY KEY (id);
119+
\d+ test_replica_identity4
120+
ALTER INDEX test_replica_identity4_pkey
121+
ATTACH PARTITION test_replica_identity4_1_pkey;
122+
\d+ test_replica_identity4
123+
105124
DROP TABLE test_replica_identity;
106125
DROP TABLE test_replica_identity2;
107126
DROP TABLE test_replica_identity3;
127+
DROP TABLE test_replica_identity4;
108128
DROP TABLE test_replica_identity_othertable;

0 commit comments

Comments
 (0)
0