From 3495f1523bc651b4c9bb5f736ce22b98933ed2e3 Mon Sep 17 00:00:00 2001 From: Nisha Moond Date: Wed, 30 Sep 2026 15:33:03 +0530 Subject: [PATCH v7] Use the relation map's index when searching deleted tuples sequentially When the index can't be used to find a recently deleted row, RelationFindDeletedTupleInfoSeq() scans the table. It looked up the relation's replica identity or primary key again to decide which columns to compare, which could disagree with the index chosen when the relation was opened. A concurrent DROP INDEX CONCURRENTLY could remove the replica identity mid-change, so the whole row was compared, and update_deleted was reported as update_missing. Removing the relcache lookups loses nothing, as the relation map entry already records the replica identity index, or failing that the primary key. The only difference is that a deferrable primary key, which cannot serve as a replica identity, is no longer used; comparing only its columns could report a deleted row with a different value as update_deleted. Pass the index from the relation map and compare its key columns only when it is the replica identity or primary key. Otherwise compare the whole row, as the publisher then uses REPLICA IDENTITY FULL. Author: Hayato Kuroda Author: Nisha Moond Reviewed-by: Vignesh C Reviewed-by: Zhijie Hou Discussion: https://postgr.es/m/CABdArM5ydwdRrpaZyK1q2p3-vY_+pnBtTmkvg_pcM=gHwmH7Kg@mail.gmail.com Discussion: https://postgr.es/m/CAA4eK1KHDofZjMRxJLHzP1Pbnkfekf7RLgR=TUJsxqmrYEKP4w@mail.gmail.com Backpatch-through: 19 --- src/backend/executor/execReplication.c | 49 ++++++++++++------ src/backend/replication/logical/worker.c | 9 ++-- src/include/executor/executor.h | 2 +- src/test/subscription/t/035_conflicts.pl | 63 ++++++++++++++++++++++++ 4 files changed, 104 insertions(+), 19 deletions(-) diff --git a/src/backend/executor/execReplication.c b/src/backend/executor/execReplication.c index dd42acc13e2..18da4fe08f6 100644 --- a/src/backend/executor/execReplication.c +++ b/src/backend/executor/execReplication.c @@ -536,6 +536,10 @@ update_most_recent_deletion_info(TupleTableSlot *scanslot, * returns the transaction ID, origin, and commit timestamp of the transaction * that deleted this tuple. * + * If 'identidxoid' is valid, it is the replica identity or primary key + * index, and only its key columns are compared. Otherwise, all columns are + * compared. + * * 'oldestxmin' acts as a cutoff transaction ID. Tuples deleted by transactions * with IDs >= 'oldestxmin' are considered recently dead and are eligible for * conflict detection. @@ -563,7 +567,8 @@ update_most_recent_deletion_info(TupleTableSlot *scanslot, * tuple was deleted most recently. */ bool -RelationFindDeletedTupleInfoSeq(Relation rel, TupleTableSlot *searchslot, +RelationFindDeletedTupleInfoSeq(Relation rel, Oid identidxoid, + TupleTableSlot *searchslot, TransactionId oldestxmin, TransactionId *delete_xid, ReplOriginId *delete_origin, @@ -572,7 +577,7 @@ RelationFindDeletedTupleInfoSeq(Relation rel, TupleTableSlot *searchslot, TupleTableSlot *scanslot; TableScanDesc scan; TypeCacheEntry **eq; - Bitmapset *indexbitmap; + Bitmapset *indexbitmap = NULL; TupleDesc desc PG_USED_FOR_ASSERTS_ONLY = RelationGetDescr(rel); Assert(equalTupleDescs(desc, searchslot->tts_tupleDescriptor)); @@ -582,21 +587,35 @@ RelationFindDeletedTupleInfoSeq(Relation rel, TupleTableSlot *searchslot, *delete_time = 0; /* - * If the relation has a replica identity key or a primary key that is - * unusable for locating deleted tuples (see - * IsIndexUsableForFindingDeletedTuple), a full table scan becomes - * necessary. In such cases, comparing the entire tuple is not required, - * since the remote tuple might not include all column values. Instead, - * the indexed columns alone are sufficient to identify the target tuple - * (see logicalrep_rel_mark_updatable). + * If the caller's replica identity key or primary key is unusable for + * locating deleted tuples (see IsIndexUsableForFindingDeletedTuple), a + * full table scan becomes necessary. In such cases, comparing the entire + * tuple is not required, since the remote tuple might not include all + * column values. Instead, the indexed columns alone are sufficient to + * identify the target tuple (see logicalrep_rel_mark_updatable). */ - indexbitmap = RelationGetIndexAttrBitmap(rel, - INDEX_ATTR_BITMAP_IDENTITY_KEY); + if (OidIsValid(identidxoid)) + { + /* The index must have been locked already */ + Relation idxrel = index_open(identidxoid, NoLock); - /* fallback to PK if no replica identity */ - if (!indexbitmap) - indexbitmap = RelationGetIndexAttrBitmap(rel, - INDEX_ATTR_BITMAP_PRIMARY_KEY); + /* + * The index may no longer be the replica identity if DROP INDEX + * CONCURRENTLY or REINDEX CONCURRENTLY ran meanwhile, but it stays + * unique and non-partial, which is all we rely on. See + * FindReplTupleInLocalRel(). + */ + Assert(idxrel->rd_index->indisunique); + Assert(heap_attisnull(idxrel->rd_indextuple, Anum_pg_index_indpred, + NULL)); + + for (int i = 0; i < idxrel->rd_index->indnkeyatts; i++) + indexbitmap = bms_add_member(indexbitmap, + idxrel->rd_index->indkey.values[i] - + FirstLowInvalidHeapAttributeNumber); + + index_close(idxrel, NoLock); + } eq = palloc0_array(TypeCacheEntry *, searchslot->tts_tupleDescriptor->natts); diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c index ab7c4ced66d..44d735cdc55 100644 --- a/src/backend/replication/logical/worker.c +++ b/src/backend/replication/logical/worker.c @@ -3409,9 +3409,12 @@ FindDeletedTupleInLocalRel(Relation localrel, delete_xid, delete_origin, delete_time); else - return RelationFindDeletedTupleInfoSeq(localrel, remoteslot, - oldestxmin, delete_xid, - delete_origin, delete_time); + return RelationFindDeletedTupleInfoSeq(localrel, + relmapentry->idxisreplident ? + localidxoid : InvalidOid, + remoteslot, oldestxmin, + delete_xid, delete_origin, + delete_time); } /* diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h index 23a09a70aa2..152a1dfa568 100644 --- a/src/include/executor/executor.h +++ b/src/include/executor/executor.h @@ -781,7 +781,7 @@ extern bool RelationFindReplTupleByIndex(Relation rel, Oid idxoid, TupleTableSlot *outslot); extern bool RelationFindReplTupleSeq(Relation rel, LockTupleMode lockmode, TupleTableSlot *searchslot, TupleTableSlot *outslot); -extern bool RelationFindDeletedTupleInfoSeq(Relation rel, +extern bool RelationFindDeletedTupleInfoSeq(Relation rel, Oid identidxoid, TupleTableSlot *searchslot, TransactionId oldestxmin, TransactionId *delete_xid, diff --git a/src/test/subscription/t/035_conflicts.pl b/src/test/subscription/t/035_conflicts.pl index 5804e38ed69..80177b214c0 100644 --- a/src/test/subscription/t/035_conflicts.pl +++ b/src/test/subscription/t/035_conflicts.pl @@ -376,6 +376,69 @@ like( .*The row to be updated was deleted locally in transaction [0-9]+ at .*/, 'update target row was deleted in tab'); +############################################################################### +# Ensure that a deferrable primary key is not used to match deleted tuples in +# a sequential table scan. Such a key cannot serve as a replica identity, so +# the whole tuple must be compared, and a deleted row that only shares the key +# value must not be reported as update_deleted. +############################################################################### + +# Create the table and publish it from node B only, so that local changes on +# node A are not sent back. Skip the initial copy, so that node A never has +# the row from node B. +$node_B->safe_psql( + 'postgres', " + CREATE TABLE tab_defer (a int, b int); + ALTER TABLE tab_defer REPLICA IDENTITY FULL; + INSERT INTO tab_defer VALUES (1, 1);"); +$node_A->safe_psql('postgres', "CREATE TABLE tab_defer (a int, b int)"); +$node_B->safe_psql('postgres', + "ALTER PUBLICATION tap_pub_B ADD TABLE tab_defer"); +$node_A->safe_psql('postgres', + "ALTER SUBSCRIPTION $subname_AB REFRESH PUBLICATION WITH (copy_data = false)" +); +$node_A->wait_for_subscription_sync($node_B, $subname_AB); + +# Disable the logical replication from node B to node A +$node_A->safe_psql('postgres', "ALTER SUBSCRIPTION $subname_AB DISABLE"); + +# Wait for the apply worker to stop +$node_A->poll_query_until('postgres', + "SELECT count(*) = 0 FROM pg_stat_activity WHERE backend_type = 'logical replication apply worker'" +); + +# The primary key is created after the conflict detection slot's xmin, so it +# cannot be used to find deleted tuples and a sequential scan is used instead. +# Then delete a local row that has the same key but a different value. +$node_A->safe_psql( + 'postgres', " + ALTER TABLE tab_defer ADD PRIMARY KEY (a) DEFERRABLE; + INSERT INTO tab_defer VALUES (1, 10); + DELETE FROM tab_defer WHERE a = 1;"); + +$node_B->safe_psql('postgres', "UPDATE tab_defer SET b = 2 WHERE a = 1;"); + +$log_location = -s $node_A->logfile; + +$node_A->safe_psql('postgres', "ALTER SUBSCRIPTION $subname_AB ENABLE;"); +$node_B->wait_for_catchup($subname_AB); + +$logfile = slurp_file($node_A->logfile(), $log_location); +like( + $logfile, + qr/conflict detected on relation "public.tab_defer": conflict=update_missing.* +.*DETAIL:.* Could not find the row to be updated: remote row \(1, 2\), replica identity full \(1, 1\)/, + 'deleted row matching only the deferrable primary key is not reported as update_deleted' +); + +# Clean up +$node_B->safe_psql('postgres', + "ALTER PUBLICATION tap_pub_B DROP TABLE tab_defer"); +$node_A->safe_psql('postgres', + "ALTER SUBSCRIPTION $subname_AB REFRESH PUBLICATION"); +$node_A->safe_psql('postgres', "DROP TABLE tab_defer"); +$node_B->safe_psql('postgres', "DROP TABLE tab_defer"); + ############################################################################### # Check that the xmin value of the conflict detection slot can be advanced when # the subscription has no tables. -- 2.34.1