From 45d51f32feb36c07c9dc2129dc47e0293a155b74 Mon Sep 17 00:00:00 2001 From: Zhijie Hou Date: Sun, 11 Oct 2026 17:18:39 +0800 Subject: [PATCH v3 1/3] Fix equality semantics in RelationFindDeletedTupleInfoSeq() When the replica identity or primary key index cannot be used to locate deleted tuples (see IsIndexUsableForFindingDeletedTuple), RelationFindDeletedTupleInfoSeq() falls back to a sequential scan that compares only the index key columns. However, it compared them using the type's default equality operator and the column's attcollation, whereas the index-based search paths use the index opclass's equality operator and the index column's collation (see build_replindex_scan_key). A replica identity index is not required to share the column's collation or the type's default operator class, so the two notions of equality can differ. The same state could then be classified as either update_deleted or update_missing depending only on whether the index happened to be usable; worse, the sequential scan could match a deleted tuple that an index scan would never have matched, or miss one that it would have matched. Fix by building a scan key on the index for the remote tuple (see build_replindex_scan_key) and comparing the key columns with the scan key's equality operators and collations. Also remove tuples_equal()'s columns parameter, which no longer has any non-NULL caller. --- src/backend/executor/execReplication.c | 113 ++++++++++++++++------- src/test/subscription/t/035_conflicts.pl | 95 +++++++++++++++++++ 2 files changed, 177 insertions(+), 31 deletions(-) diff --git a/src/backend/executor/execReplication.c b/src/backend/executor/execReplication.c index 18da4fe08f6..57090993c6b 100644 --- a/src/backend/executor/execReplication.c +++ b/src/backend/executor/execReplication.c @@ -39,7 +39,9 @@ static bool tuples_equal(TupleTableSlot *slot1, TupleTableSlot *slot2, - TypeCacheEntry **eq, Bitmapset *columns); + TypeCacheEntry **eq); +static bool repl_index_key_equal(Relation idxrel, TupleTableSlot *slot1, + TupleTableSlot *slot2, ScanKey skey); /* * Setup a ScanKey for a search in the relation 'rel' for a tuple 'key' that @@ -227,7 +229,7 @@ retry: if (eq == NULL) eq = palloc0_array(TypeCacheEntry *, outslot->tts_tupleDescriptor->natts); - if (!tuples_equal(outslot, searchslot, eq, NULL)) + if (!tuples_equal(outslot, searchslot, eq)) continue; } @@ -283,13 +285,9 @@ retry: /* * Compare the tuples in the slots by checking if they have equal values. - * - * If 'columns' is not null, only the columns specified within it will be - * considered for the equality check, ignoring all other columns. */ static bool -tuples_equal(TupleTableSlot *slot1, TupleTableSlot *slot2, - TypeCacheEntry **eq, Bitmapset *columns) +tuples_equal(TupleTableSlot *slot1, TupleTableSlot *slot2, TypeCacheEntry **eq) { int attrnum; @@ -314,14 +312,6 @@ tuples_equal(TupleTableSlot *slot1, TupleTableSlot *slot2, if (att->attisdropped || att->attgenerated) continue; - /* - * Ignore columns that are not listed for checking. - */ - if (columns && - !bms_is_member(att->attnum - FirstLowInvalidHeapAttributeNumber, - columns)) - continue; - /* * If one value is NULL and other is not, then they are certainly not * equal @@ -358,6 +348,51 @@ tuples_equal(TupleTableSlot *slot1, TupleTableSlot *slot2, return true; } +/* + * Compare the key columns of index 'idxrel' between the two slots, using the + * equality operators and collations of the scan key 'skey' built by + * build_replindex_scan_key(). + */ +static bool +repl_index_key_equal(Relation idxrel, TupleTableSlot *slot1, + TupleTableSlot *slot2, ScanKey skey) +{ + Assert(slot1->tts_tupleDescriptor->natts == + slot2->tts_tupleDescriptor->natts); + + for (int i = 0; i < idxrel->rd_index->indnkeyatts; i++) + { + AttrNumber keycol = idxrel->rd_index->indkey.values[i]; + Datum value1; + Datum value2; + bool isnull1; + bool isnull2; + + value1 = slot_getattr(slot1, keycol, &isnull1); + value2 = slot_getattr(slot2, keycol, &isnull2); + + /* + * If one value is NULL and other is not, then they are certainly not + * equal + */ + if (isnull1 != isnull2) + return false; + + /* + * If both are NULL, they can be considered equal. + */ + if (isnull1 || isnull2) + continue; + + if (!DatumGetBool(FunctionCall2Coll(&skey[i].sk_func, + skey[i].sk_collation, + value1, value2))) + return false; + } + + return true; +} + /* * Search the relation 'rel' for tuple using the sequential scan. * @@ -398,7 +433,7 @@ retry: /* Try to find the tuple */ while (table_scan_getnextslot(scan, ForwardScanDirection, scanslot)) { - if (!tuples_equal(scanslot, searchslot, eq, NULL)) + if (!tuples_equal(scanslot, searchslot, eq)) continue; found = true; @@ -537,8 +572,8 @@ update_most_recent_deletion_info(TupleTableSlot *scanslot, * 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. + * index, and only its key columns are compared, using the index's equality + * semantics (see repl_index_key_equal). 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 @@ -576,8 +611,9 @@ RelationFindDeletedTupleInfoSeq(Relation rel, Oid identidxoid, { TupleTableSlot *scanslot; TableScanDesc scan; - TypeCacheEntry **eq; - Bitmapset *indexbitmap = NULL; + TypeCacheEntry **eq = NULL; + ScanKeyData skey[INDEX_MAX_KEYS]; + Relation idxrel = NULL; TupleDesc desc PG_USED_FOR_ASSERTS_ONLY = RelationGetDescr(rel); Assert(equalTupleDescs(desc, searchslot->tts_tupleDescriptor)); @@ -593,11 +629,16 @@ RelationFindDeletedTupleInfoSeq(Relation rel, Oid identidxoid, * 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). + * + * The comparison must use the index's equality semantics (the opclass's + * equality operator and the index column's collation) so that the result + * matches what RelationFindDeletedTupleInfoByIndex() would have reported + * had the index been usable. */ if (OidIsValid(identidxoid)) { /* The index must have been locked already */ - Relation idxrel = index_open(identidxoid, NoLock); + idxrel = index_open(identidxoid, NoLock); /* * The index may no longer be the replica identity if DROP INDEX @@ -609,15 +650,17 @@ RelationFindDeletedTupleInfoSeq(Relation rel, Oid identidxoid, 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); + /* + * Build a scan key from the remote tuple, carrying the index's + * per-column equality operators and collations for + * repl_index_key_equal() to compare the key columns below. + */ + (void) build_replindex_scan_key(skey, rel, idxrel, searchslot); + } + else + { + eq = palloc0_array(TypeCacheEntry *, searchslot->tts_tupleDescriptor->natts); } - - eq = palloc0_array(TypeCacheEntry *, searchslot->tts_tupleDescriptor->natts); /* * Start a heap scan using SnapshotAny to identify dead tuples that are @@ -634,7 +677,12 @@ RelationFindDeletedTupleInfoSeq(Relation rel, Oid identidxoid, /* Try to find the tuple */ while (table_scan_getnextslot(scan, ForwardScanDirection, scanslot)) { - if (!tuples_equal(scanslot, searchslot, eq, indexbitmap)) + if (idxrel) + { + if (!repl_index_key_equal(idxrel, scanslot, searchslot, skey)) + continue; + } + else if (!tuples_equal(scanslot, searchslot, eq)) continue; update_most_recent_deletion_info(scanslot, oldestxmin, delete_xid, @@ -644,6 +692,9 @@ RelationFindDeletedTupleInfoSeq(Relation rel, Oid identidxoid, table_endscan(scan); ExecDropSingleTupleTableSlot(scanslot); + if (idxrel) + index_close(idxrel, NoLock); + return *delete_time != 0; } @@ -707,7 +758,7 @@ RelationFindDeletedTupleInfoByIndex(Relation rel, Oid idxoid, if (eq == NULL) eq = palloc0_array(TypeCacheEntry *, scanslot->tts_tupleDescriptor->natts); - if (!tuples_equal(scanslot, searchslot, eq, NULL)) + if (!tuples_equal(scanslot, searchslot, eq)) continue; } diff --git a/src/test/subscription/t/035_conflicts.pl b/src/test/subscription/t/035_conflicts.pl index 80177b214c0..246991224c7 100644 --- a/src/test/subscription/t/035_conflicts.pl +++ b/src/test/subscription/t/035_conflicts.pl @@ -439,6 +439,101 @@ $node_A->safe_psql('postgres', $node_A->safe_psql('postgres', "DROP TABLE tab_defer"); $node_B->safe_psql('postgres', "DROP TABLE tab_defer"); +############################################################################### +# Check that when the replica identity index cannot be used to find deleted +# tuples (see IsIndexUsableForFindingDeletedTuple), the sequential scan +# compares the key columns using the index's equality semantics (the opclass's +# equality operator and the index column's collation), the same as the index +# scan would. This requires ICU for a nondeterministic collation. +# +# The replica identity index uses a case-insensitive collation that differs +# from the key column's attcollation ("C"), so that the two notions of +# equality diverge: 'abc' and 'ABC' are equal under the index's collation but +# not under the column's. The index is created after the conflict detection +# slot's xmin is pinned, so the sequential scan is used; the locally deleted +# row 'ABC' must still be reported as update_deleted for the publisher's key +# 'abc', exactly as if the index had been used. +############################################################################### +if ($ENV{with_icu} eq 'yes') +{ + # Publisher table holding the lowercase row. + $node_B->safe_psql( + 'postgres', qq[ + CREATE TABLE tab_coll (k text PRIMARY KEY, v int); + INSERT INTO tab_coll VALUES ('abc', 1); + ALTER PUBLICATION tap_pub_B ADD TABLE tab_coll; + ]); + + $node_A->safe_psql( + 'postgres', qq[ + CREATE COLLATION coll_ci (provider = icu, locale = 'und-u-ks-level2', deterministic = false); + CREATE TABLE tab_coll (k text COLLATE "C" NOT NULL, v int); + ]); + + $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); + + # Pin the conflict detection slot's xmin: hold an open transaction with an + # assigned transaction ID so that oldest_nonremovable_xid cannot advance + # past it. An index created afterwards has a pg_index.xmin greater than + # the slot's xmin and is therefore not used to find deleted tuples, + # forcing the sequential scan. + my $holder = $node_A->background_psql('postgres'); + $holder->query_safe('BEGIN'); + my $pinned_xid = $holder->query_safe('SELECT txid_current()'); + + ok( $node_A->poll_query_until( + 'postgres', + "SELECT xmin::text = '$pinned_xid' FROM pg_replication_slots WHERE slot_name = 'pg_conflict_detection'" + ), + "conflict detection slot xmin pinned at $pinned_xid"); + + # Late replica identity index with the case-insensitive collation. + $node_A->safe_psql( + 'postgres', qq[ + CREATE UNIQUE INDEX tab_coll_ri ON tab_coll (k COLLATE coll_ci); + ALTER TABLE tab_coll REPLICA IDENTITY USING INDEX tab_coll_ri; + ]); + + # Insert and locally delete a row that is equal to the publisher's key + # only under the index's collation. + $node_A->safe_psql( + 'postgres', qq[ + INSERT INTO tab_coll VALUES ('ABC', 1); + DELETE FROM tab_coll WHERE k = 'ABC'; + ]); + + $log_location = -s $node_A->logfile; + + # The publisher updates its row; the old key sent is 'abc'. + $node_B->safe_psql('postgres', "UPDATE tab_coll SET v = 2 WHERE k = 'abc'"); + + $node_B->wait_for_catchup($subname_AB); + + $logfile = slurp_file($node_A->logfile(), $log_location); + + like( + $logfile, + qr/conflict detected on relation "public.tab_coll": conflict=update_deleted.* +.*DETAIL:.* Could not find the row to be updated: remote row \(abc, 2\), replica identity \(k\)=\(abc\). +.*The row to be updated was deleted locally in transaction [0-9]+ at .*/, + 'update_deleted detected via sequential scan with index equality semantics' + ); + + # Clean up + $holder->query_safe('COMMIT'); + ok($holder->quit, 'release the pinned transaction'); + $node_B->safe_psql('postgres', + "ALTER PUBLICATION tap_pub_B DROP TABLE tab_coll"); + $node_A->safe_psql('postgres', + "ALTER SUBSCRIPTION $subname_AB REFRESH PUBLICATION"); + $node_A->safe_psql('postgres', "DROP TABLE tab_coll"); + $node_B->safe_psql('postgres', "DROP TABLE tab_coll"); + $node_A->safe_psql('postgres', "DROP COLLATION coll_ci"); +} + ############################################################################### # Check that the xmin value of the conflict detection slot can be advanced when # the subscription has no tables. -- 2.34.1