Subject: Re: [BUG?] check_exclusion_or_unique_constraint false negative

From: Amit Kapila <[email-masked]>
Date: Fri, 2 Aug 2024 10:26:40 +0530

On Thu, Aug 1, 2024 at 2:55 PM Michail Nikolaev
<[email-masked]> wrote:
>
> > Thanks for pointing out the issue!
>
> Thanks for your attention!
>
> > IIUC, the issue can happen when two concurrent transactions using DirtySnapshot access
> > the same tuples, which is not specific to the parallel apply
>
> Not exactly, it happens for any DirtySnapshot scan over a B-tree index with some other transaction updating the same index page (even using the MVCC snapshot).
>
> So, logical replication related scenario looks like this:
>
> * subscriber worker receives a tuple update\delete from the publisher
> * it calls RelationFindReplTupleByIndex to find the tuple in the local table
> * some other transaction updates the tuple in the local table (on subscriber side) in parallel
> * RelationFindReplTupleByIndex may not find the tuple because it uses DirtySnapshot
> * update\delete is lost
>
> Parallel apply mode looks like more dangerous because it uses multiple workers on the subscriber side, so the probability of the issue is higher.
> In that case, "some other transaction" is just another worker applying changes of different transaction in parallel.
>

I think it is rather less likely or not possible in a parallel apply
case because such conflicting updates (updates on the same tuple)
should be serialized at the publisher itself. So one of the updates
will be after the commit that has the second update.

I haven't tried the test based on your description of the general
problem with DirtySnapshot scan. In case of logical replication, we
will LOG update_missing type of conflict and the user may need to take
some manual action based on that. I have not tried a test so I could
be wrong as well. I am not sure we can do anything specific to logical
replication for this but feel free to suggest if you have ideas to
solve this problem in general or specific to logical replication.

-- 
With Regards,
Amit Kapila.

---

From: Michail Nikolaev <[email-masked]>
Date: Fri, 2 Aug 2024 19:08:39 +0200

Hello, Amit!

> will be after the commit that has the second update.

Glad to hear! But anyway, such logic looks very fragile to me.

> some manual action based on that.

Current it is just DEBUG1, so it will be probably missed by the user.

> * XXX should this be promoted to ereport(LOG) perhaps?
> */
> elog(DEBUG1,
> "logical replication did not find row to be updated "
> "in replication target relation \"%s\"",
> RelationGetRelationName(localrel));
> }

> I have not tried a test so I could

I've implemented a solution to address the problem more generally, attached
the patch (and also the link [1]).

Here's a summary of the changes:

* For each tuple skipped because it was deleted, we now accumulate the
maximum xmax.
* Before the scan begins, we store the value of the latest completed
transaction.
* If no tuples are found in the index, we check the max(xmax) value. If
this value is newer than the latest completed transaction stored before the
scan, it indicates that a tuple was deleted by another transaction after
the scan started. To ensure all tuples are correctly processed we then
rescan the index.

Also added a test case to cover this scenario using the new injection point
mechanism and
updated the b-tree index documentation to include a description of this
case.

I'll add this into the next commitfest.

Best regards,
Mikhail.

[1]:
https://github.com/postgres/postgres/compare/master...michail-nikolaev:postgres:concurrent_unique

---

From: Amit Kapila <[email-masked]>
Date: Mon, 5 Aug 2024 15:46:20 +0530

> > RelationGetRelationName(localrel));

Right, but we are extending this functionality to detect and resolve
such conflicts [1][2]. I am hoping after that such updates won't be
missed.

[1] - https://commitfest.postgresql.org/49/5064/
[2] - https://commitfest.postgresql.org/49/5021/

-- 
With Regards,
Amit Kapila.

---

From: Michail Nikolaev <[email-masked]>
Date: Mon, 5 Aug 2024 13:11:50 +0200

Hello!

> such conflicts [1][2]. I am hoping after that such updates won't be

Yes, this is a nice feature. However, without the DirtySnapshot index scan
fix, it will fail in numerous instances, especially in master-master
replication.

The update_missing feature is helpful in this case, but it is still not the
correct event because a real tuple exists, and we should receive
update_differ instead. As a result, some conflict resolution systems may
malfunction. For example, if the resolution method is set to apply_or_skip,
it will insert the new row, causing two rows to exist. This system is quite
fragile, and I am sure there are many more complicated scenarios that could
arise.
Best regards,
Mikhail.

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Mon, 12 Aug 2024 03:32:32 +0000

Hi,

Thanks for reporting the issue !

I tried to reproduce this in logical replication but failed. If possible,
could you please share some steps to reproduce it in logicalrep context ?

In my test, if the tuple is updated and new tuple is in the same page,
heapam_index_fetch_tuple should find the new tuple using HOT chain. So, it's a
bit unclear to me how the updated tuple is missing. Maybe I missed some other
conditions for this issue.

It would be better if we can reproduce this by adding some breakpoints using
gdb, which may help us to write a tap test using injection point to reproduce
this reliably. I see the tap test you shared used pgbench to reproduce this,
it works, but It would be great if we can analyze the issue more deeply by
debugging the code.

And I have few questions related the steps you shared:

> * Session 1 reads a B-tree page using SnapshotDirty and copies item X to the buffer.
> * Session 2 updates item X, inserting a new TID Y into the same page.
> * Session 2 commits its transaction.
> * Session 1 starts to fetch from the heap and tries to fetch X, but it was
>   already deleted by session 2. So, it goes to the B-tree for the next TID.
> * The B-tree goes to the next page, skipping Y.
> * Therefore, the search finds nothing, but tuple Y is still alive.

I am wondering at which point should the update happen ? should it happen after
calling index_getnext_tid and before index_fetch_heap ? It would be great if
you could give more details in above steps. Thanks !

Best Regards,
Hou zj

---

From: Michail Nikolaev <[email-masked]>
Date: Mon, 12 Aug 2024 13:11:26 +0200

Hello, Hou zj!

> conditions for this issue.

Yeah, I think the pgbench-based reproducer may also cause page splits in
btree.
But we may add an index to the table to disable HOT.

I have attached a reproducer for this case using a spec and injection
points.

I hope it helps, check the attached files.

Best regards,
Mikhail.
From e3c1beb4d2739fb3b1cb7e068a7ef91b0da61fc6 Mon Sep 17 00:00:00 2001
From: nkey <[email-masked]>
Date: Mon, 12 Aug 2024 13:07:15 +0200
Subject: [PATCH v2 2/2] additional test spec to reproduce dirty snapshot scan
 issue

---
 src/backend/access/index/indexam.c            |  8 ++++
 src/backend/executor/execIndexing.c           |  1 +
 src/test/modules/injection_points/Makefile    |  2 +-
 .../expected/dirty_index_scan.out             | 39 +++++++++++++++++
 src/test/modules/injection_points/meson.build |  1 +
 .../specs/dirty_index_scan.spec               | 43 +++++++++++++++++++
 6 files changed, 93 insertions(+), 1 deletion(-)
 create mode 100644 src/test/modules/injection_points/expected/dirty_index_scan.out
 create mode 100644 src/test/modules/injection_points/specs/dirty_index_scan.spec

diff --git a/src/backend/access/index/indexam.c b/src/backend/access/index/indexam.c
index dcd04b813d..78b3a58b3b 100644
--- a/src/backend/access/index/indexam.c
+++ b/src/backend/access/index/indexam.c
@@ -57,6 +57,7 @@
 #include "utils/ruleutils.h"
 #include "utils/snapmgr.h"
 #include "utils/syscache.h"
+#include "utils/injection_point.h"

 /* ----------------------------------------------------------------
@@ -694,6 +695,13 @@ index_getnext_slot(IndexScanDesc scan, ScanDirection direction, TupleTableSlot *
 		 * the index.
 		 */
 		Assert(ItemPointerIsValid(&scan->xs_heaptid));
+#ifdef USE_INJECTION_POINTS
+		if (!IsCatalogRelationOid(scan->indexRelation->rd_id))
+		{
+			INJECTION_POINT("index_getnext_slot_before_fetch");
+		}
+#endif
+
 		if (index_fetch_heap(scan, slot))
 			return true;
 	}
diff --git a/src/backend/executor/execIndexing.c b/src/backend/executor/execIndexing.c
index 45767b4e20..479f145d99 100644
--- a/src/backend/executor/execIndexing.c
+++ b/src/backend/executor/execIndexing.c
@@ -775,6 +775,7 @@ check_exclusion_or_unique_constraint(Relation heap, Relation index,
 	 * May have to restart scan from this point if a potential conflict is
 	 * found.
 	 */
+	INJECTION_POINT("check_exclusion_or_unique_constraint_before_index_scan");
 retry:
 	conflict = false;
 	found_self = false;
diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile
index 2ffd2f77ed..981dff4e6f 100644
--- a/src/test/modules/injection_points/Makefile
+++ b/src/test/modules/injection_points/Makefile
@@ -9,7 +9,7 @@ PGFILEDESC = "injection_points - facility for injection points"
 REGRESS = injection_points
 REGRESS_OPTS = --dlpath=$(top_builddir)/src/test/regress
 
-ISOLATION = inplace
+ISOLATION = inplace dirty_index_scan
 
 # The injection points are cluster-wide, so disable installcheck
 NO_INSTALLCHECK = 1
diff --git a/src/test/modules/injection_points/expected/dirty_index_scan.out b/src/test/modules/injection_points/expected/dirty_index_scan.out
new file mode 100644
index 0000000000..249e69cb72
--- /dev/null
+++ b/src/test/modules/injection_points/expected/dirty_index_scan.out
@@ -0,0 +1,39 @@
+Parsed test spec with 3 sessions
+
+starting permutation: s1_s1 s3_s1 s2_s1 s3_s2
+injection_points_attach
+-----------------------
+                       
+(1 row)
+
+step s1_s1: INSERT INTO test.tbl VALUES(42, 1) on conflict(i) do update set n = EXCLUDED.n + 1; <waiting ...>
+step s3_s1: 
+	SELECT injection_points_wakeup('check_exclusion_or_unique_constraint_before_index_scan');
+	SELECT injection_points_detach('check_exclusion_or_unique_constraint_before_index_scan');
+
+injection_points_wakeup
+-----------------------
+                       
+(1 row)
+
+injection_points_detach
+-----------------------
+                       
+(1 row)
+
+step s2_s1: UPDATE test.tbl SET n = n + 1 WHERE i = 42;
+step s3_s2: 
+	SELECT injection_points_wakeup('index_getnext_slot_before_fetch');
+	SELECT injection_points_detach('index_getnext_slot_before_fetch');
+
+injection_points_wakeup
+-----------------------
+                       
+(1 row)
+
+injection_points_detach
+-----------------------
+                       
+(1 row)
+
+step s1_s1: <... completed>
diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build
index 3c23c14d81..4969952797 100644
--- a/src/test/modules/injection_points/meson.build
+++ b/src/test/modules/injection_points/meson.build
@@ -40,6 +40,7 @@ tests += {
   'isolation': {
     'specs': [
       'inplace',
+      'dirty_index_scan',
     ],
   },
 }
diff --git a/src/test/modules/injection_points/specs/dirty_index_scan.spec b/src/test/modules/injection_points/specs/dirty_index_scan.spec
new file mode 100644
index 0000000000..17c5ec37e6
--- /dev/null
+++ b/src/test/modules/injection_points/specs/dirty_index_scan.spec
@@ -0,0 +1,43 @@
+setup
+{
+	CREATE EXTENSION injection_points;
+	CREATE SCHEMA test;
+	CREATE UNLOGGED TABLE test.tbl(i int primary key, n int);
+	CREATE INDEX tbl_n_idx ON test.tbl(n);
+	INSERT INTO test.tbl VALUES(42,1);
+}
+
+teardown
+{
+	DROP SCHEMA test CASCADE;
+	DROP EXTENSION injection_points;
+}
+
+session s1
+setup	{
+	SELECT injection_points_set_local();
+	SELECT injection_points_attach('check_exclusion_or_unique_constraint_no_conflict', 'error');
+	SELECT injection_points_attach('check_exclusion_or_unique_constraint_before_index_scan', 'wait');
+	SELECT injection_points_attach('index_getnext_slot_before_fetch', 'wait');
+}
+
+step s1_s1	{ INSERT INTO test.tbl VALUES(42, 1) on conflict(i) do update set n = EXCLUDED.n + 1; }
+
+session s2
+step s2_s1	{ UPDATE test.tbl SET n = n + 1 WHERE i = 42; }
+
+session s3
+step s3_s1		{
+	SELECT injection_points_wakeup('check_exclusion_or_unique_constraint_before_index_scan');
+	SELECT injection_points_detach('check_exclusion_or_unique_constraint_before_index_scan');
+}
+step s3_s2		{
+	SELECT injection_points_wakeup('index_getnext_slot_before_fetch');
+	SELECT injection_points_detach('index_getnext_slot_before_fetch');
+}
+
+permutation
+	s1_s1
+	s3_s1
+	s2_s1
+	s3_s2
\ No newline at end of file
-- 
2.34.1

From b639379c393c70ca322bab57222513e71c96ad78 Mon Sep 17 00:00:00 2001
From: nkey <[email-masked]>
Date: Fri, 2 Aug 2024 16:20:32 +0200
Subject: [PATCH v2 1/2] fix for lost record in case of DirtySnapshot index
 scans + docs + test

---
 contrib/pgstattuple/pgstattuple.c             |  2 +-
 src/backend/access/heap/heapam_handler.c      |  2 +-
 src/backend/access/heap/heapam_visibility.c   | 20 +++++++-
 src/backend/access/nbtree/README              | 10 ++++
 src/backend/access/nbtree/nbtinsert.c         |  2 +-
 src/backend/access/transam/varsup.c           | 11 +++++
 src/backend/executor/execIndexing.c           | 29 +++++++++++-
 src/backend/executor/execReplication.c        | 26 +++++++++-
 src/backend/replication/logical/origin.c      |  2 +-
 src/include/access/transam.h                  | 16 +++++++
 src/include/utils/snapmgr.h                   |  8 +++-
 src/include/utils/snapshot.h                  |  5 +-
 .../test_misc/t/006_dirty_index_scan.pl       | 47 +++++++++++++++++++
 13 files changed, 168 insertions(+), 12 deletions(-)
 create mode 100644 src/test/modules/test_misc/t/006_dirty_index_scan.pl

diff --git a/contrib/pgstattuple/pgstattuple.c b/contrib/pgstattuple/pgstattuple.c
index 3bd8b96197..4d1b469d8e 100644
--- a/contrib/pgstattuple/pgstattuple.c
+++ b/contrib/pgstattuple/pgstattuple.c
@@ -332,7 +332,7 @@ pgstat_heap(Relation rel, FunctionCallInfo fcinfo)
 	scan = table_beginscan_strat(rel, SnapshotAny, 0, NULL, true, false);
 	hscan = (HeapScanDesc) scan;
 
-	InitDirtySnapshot(SnapshotDirty);
+	InitDirtySnapshot(SnapshotDirty, NULL);
 
 	nblocks = hscan->rs_nblocks;	/* # blocks to be scanned */
 
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 6f8b1b7929..7ea4b205d5 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -404,7 +404,7 @@ tuple_lock_retry:
 			 *
 			 * Loop here to deal with updated or busy tuples
 			 */
-			InitDirtySnapshot(SnapshotDirty);
+			InitDirtySnapshot(SnapshotDirty, NULL);
 			for (;;)
 			{
 				if (ItemPointerIndicatesMovedPartitions(tid))
diff --git a/src/backend/access/heap/heapam_visibility.c b/src/backend/access/heap/heapam_visibility.c
index 9243feed01..91de5dcea1 100644
--- a/src/backend/access/heap/heapam_visibility.c
+++ b/src/backend/access/heap/heapam_visibility.c
@@ -719,6 +719,12 @@ HeapTupleSatisfiesUpdate(HeapTuple htup, CommandId curcid,
 		return TM_Deleted;		/* deleted by other */
 }
 
+inline static void UpdateDirtyMaxXmax(Snapshot snapshot, TransactionId xmax)
+{
+	if (snapshot->xip != NULL)
+		snapshot->xip[0] = TransactionIdNewer(xmax, snapshot->xip[0]);
+}
+
 /*
  * HeapTupleSatisfiesDirty
  *		True iff heap tuple is valid including effects of open transactions.
@@ -737,7 +743,9 @@ HeapTupleSatisfiesUpdate(HeapTuple htup, CommandId curcid,
  * Similarly for snapshot->xmax and the tuple's xmax.  If the tuple was
  * inserted speculatively, meaning that the inserter might still back down
  * on the insertion without aborting the whole transaction, the associated
- * token is also returned in snapshot->speculativeToken.
+ * token is also returned in snapshot->speculativeToken. If xip is != NULL
+ * xip[0] may be set to xid of deleter if it newer than previously store
+ * value.
  */
 static bool
 HeapTupleSatisfiesDirty(HeapTuple htup, Snapshot snapshot,
@@ -750,6 +758,10 @@ HeapTupleSatisfiesDirty(HeapTuple htup, Snapshot snapshot,
 
 	snapshot->xmin = snapshot->xmax = InvalidTransactionId;
 	snapshot->speculativeToken = 0;
+	/*
+	 * We intentionally keep snapshot->xip values unchanged as they should
+	 * be reset by logic out of the single heap fetch.
+	 */
 
 	if (!HeapTupleHeaderXminCommitted(tuple))
 	{
@@ -870,6 +882,7 @@ HeapTupleSatisfiesDirty(HeapTuple htup, Snapshot snapshot,
 	{
 		if (HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask))
 			return true;
+		UpdateDirtyMaxXmax(snapshot, HeapTupleHeaderGetRawXmax(tuple));
 		return false;			/* updated by other */
 	}
 
@@ -893,7 +906,10 @@ HeapTupleSatisfiesDirty(HeapTuple htup, Snapshot snapshot,
 			return true;
 		}
 		if (TransactionIdDidCommit(xmax))
+		{
+			UpdateDirtyMaxXmax(snapshot, xmax);
 			return false;
+		}
 		/* it must have aborted or crashed */
 		return true;
 	}
@@ -902,6 +918,7 @@ HeapTupleSatisfiesDirty(HeapTuple htup, Snapshot snapshot,
 	{
 		if (HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask))
 			return true;
+		UpdateDirtyMaxXmax(snapshot, HeapTupleHeaderGetRawXmax(tuple));
 		return false;
 	}
 
@@ -931,6 +948,7 @@ HeapTupleSatisfiesDirty(HeapTuple htup, Snapshot snapshot,
 
 	SetHintBits(tuple, buffer, HEAP_XMAX_COMMITTED,
 				HeapTupleHeaderGetRawXmax(tuple));
+	UpdateDirtyMaxXmax(snapshot, HeapTupleHeaderGetRawXmax(tuple));
 	return false;				/* updated by other */
 }
 
diff --git a/src/backend/access/nbtree/README b/src/backend/access/nbtree/README
index 52e646c7f7..6de72a29ca 100644
--- a/src/backend/access/nbtree/README
+++ b/src/backend/access/nbtree/README
@@ -489,6 +489,16 @@ on the leaf page at all when the page's LSN has changed.  (That won't work
 with an unlogged index, so for now we don't ever apply the "don't hold
 onto pin" optimization there.)
 
+Despite the locking protocol in place, it is still possible to receive an
+incorrect result during non-MVCC scans. This issue can occur if a concurrent
+transaction deletes a tuple and inserts a new tuple with a new TID in the
+same page. If the scan has already visited the page and cached its content
+in the buffer cache, it might skip the old tuple due to deletion and miss
+the new tuple because of the cache. This is a known limitation of the
+SnapshotDirty and SnapshotAny non-MVCC scans. However, for SnapshotDirty,
+it is possible to work around this limitation by using the returned max(xmax)
+to compare it with the latest committed transaction before the scan started.
+
 Fastpath For Index Insertion
 ----------------------------
 
diff --git a/src/backend/access/nbtree/nbtinsert.c b/src/backend/access/nbtree/nbtinsert.c
index 7e8902e48c..943aee087a 100644
--- a/src/backend/access/nbtree/nbtinsert.c
+++ b/src/backend/access/nbtree/nbtinsert.c
@@ -427,7 +427,7 @@ _bt_check_unique(Relation rel, BTInsertState insertstate, Relation heapRel,
 	/* Assume unique until we find a duplicate */
 	*is_unique = true;
 
-	InitDirtySnapshot(SnapshotDirty);
+	InitDirtySnapshot(SnapshotDirty, NULL);
 
 	page = BufferGetPage(insertstate->buf);
 	opaque = BTPageGetOpaque(page);
diff --git a/src/backend/access/transam/varsup.c b/src/backend/access/transam/varsup.c
index fb6a86afcb..52109635b4 100644
--- a/src/backend/access/transam/varsup.c
+++ b/src/backend/access/transam/varsup.c
@@ -296,6 +296,17 @@ ReadNextFullTransactionId(void)
 	return fullXid;
 }
 
+FullTransactionId ReadLastCompletedFullTransactionId(void)
+{
+	FullTransactionId fullXid;
+
+	LWLockAcquire(XidGenLock, LW_SHARED);
+	fullXid = TransamVariables->latestCompletedXid;
+	LWLockRelease(XidGenLock);
+
+	return fullXid;
+}
+
 /*
  * Advance nextXid to the value after a given xid.  The epoch is inferred.
  * This must only be called during recovery or from two-phase start-up code.
diff --git a/src/backend/executor/execIndexing.c b/src/backend/executor/execIndexing.c
index 9f05b3654c..45767b4e20 100644
--- a/src/backend/executor/execIndexing.c
+++ b/src/backend/executor/execIndexing.c
@@ -115,6 +115,7 @@
 #include "nodes/nodeFuncs.h"
 #include "storage/lmgr.h"
 #include "utils/snapmgr.h"
+#include "utils/injection_point.h"
 
 /* waitMode argument to check_exclusion_or_unique_constraint() */
 typedef enum
@@ -702,6 +703,8 @@ check_exclusion_or_unique_constraint(Relation heap, Relation index,
 	IndexScanDesc index_scan;
 	ScanKeyData scankeys[INDEX_MAX_KEYS];
 	SnapshotData DirtySnapshot;
+	TransactionId maxXmax,
+				  latestCompletedXid;
 	int			i;
 	bool		conflict;
 	bool		found_self;
@@ -738,9 +741,10 @@ check_exclusion_or_unique_constraint(Relation heap, Relation index,
 
 	/*
 	 * Search the tuples that are in the index for any violations, including
-	 * tuples that aren't visible yet.
+	 * tuples that aren't visible yet. Also, detect cases index scan skip the
+	 * tuple in case of parallel update after index page content was cached.
 	 */
-	InitDirtySnapshot(DirtySnapshot);
+	InitDirtySnapshot(DirtySnapshot, &maxXmax);
 
 	for (i = 0; i < indnkeyatts; i++)
 	{
@@ -774,6 +778,12 @@ check_exclusion_or_unique_constraint(Relation heap, Relation index,
 retry:
 	conflict = false;
 	found_self = false;
+	/*
+	 * Each time we retry - remember last completed transaction before start
+	 * of the scan. Aso reset maxXmax.
+	 */
+	latestCompletedXid = XidFromFullTransactionId(ReadLastCompletedFullTransactionId());
+	maxXmax = InvalidTransactionId;
 	index_scan = index_beginscan(heap, index, &DirtySnapshot, indnkeyatts, 0);
 	index_rescan(index_scan, scankeys, indnkeyatts, NULL, 0);
 
@@ -889,6 +899,19 @@ retry:
 	}
 
 	index_endscan(index_scan);
+	/*
+	 * Check for the case when index scan fetched records before some other
+	 * transaction deleted tuple and inserted a new one.
+	 */
+	if (!conflict && TransactionIdIsValid(maxXmax) && !TransactionIdIsCurrentTransactionId(maxXmax))
+	{
+		/*
+		 * If we have skipped some tuple because it was deleted, but deletion happened after
+		 * start of the index scan - retry to be sure.
+		 */
+		if (TransactionIdPrecedes(latestCompletedXid, maxXmax))
+			goto retry;
+	}
 
 	/*
 	 * Ordinarily, at this point the search should have found the originally
@@ -902,6 +925,8 @@ retry:
 
 	ExecDropSingleTupleTableSlot(existing_slot);
 
+	if (!conflict)
+		INJECTION_POINT("check_exclusion_or_unique_constraint_no_conflict");
 	return !conflict;
 }
 
diff --git a/src/backend/executor/execReplication.c b/src/backend/executor/execReplication.c
index d0a89cd577..fbddb6442b 100644
--- a/src/backend/executor/execReplication.c
+++ b/src/backend/executor/execReplication.c
@@ -183,6 +183,8 @@ RelationFindReplTupleByIndex(Relation rel, Oid idxoid,
 	IndexScanDesc scan;
 	SnapshotData snap;
 	TransactionId xwait;
+	TransactionId maxXmax,
+				  latestCompletedXid;
 	Relation	idxrel;
 	bool		found;
 	TypeCacheEntry **eq = NULL;
@@ -193,7 +195,7 @@ RelationFindReplTupleByIndex(Relation rel, Oid idxoid,
 
 	isIdxSafeToSkipDuplicates = (GetRelationIdentityOrPK(rel) == idxoid);
 
-	InitDirtySnapshot(snap);
+	InitDirtySnapshot(snap, &maxXmax);
 
 	/* Build scan key. */
 	skey_attoff = build_replindex_scan_key(skey, rel, idxrel, searchslot);
@@ -203,6 +205,12 @@ RelationFindReplTupleByIndex(Relation rel, Oid idxoid,
 
 retry:
 	found = false;
+	/*
+	 * Each time we retry - remember last completed transaction before start
+ 	 * of the scan. Aso reset maxXmax.
+ 	 */
+	maxXmax = InvalidTransactionId;
+	latestCompletedXid = XidFromFullTransactionId(ReadLastCompletedFullTransactionId());
 
 	index_rescan(scan, skey, skey_attoff, NULL, 0);
 
@@ -242,6 +250,20 @@ retry:
 		break;
 	}
 
+	/*
+	 * Check for the case when index scan fetched records before some other
+	 * transaction deleted tuple and inserted a new one.
+	 */
+	if (!found && TransactionIdIsValid(maxXmax) && !TransactionIdIsCurrentTransactionId(maxXmax))
+	{
+		/*
+		 * If we have skipped some tuple because it was deleted, but deletion happened after
+		 * start of the index scan - retry to be sure.
+		 */
+		if (TransactionIdPrecedes(latestCompletedXid, maxXmax))
+			goto retry;
+	}
+
 	/* Found tuple, try to lock it in the lockmode. */
 	if (found)
 	{
@@ -391,7 +413,7 @@ RelationFindReplTupleSeq(Relation rel, LockTupleMode lockmode,
 	eq = palloc0(sizeof(*eq) * outslot->tts_tupleDescriptor->natts);
 
 	/* Start a heap scan. */
-	InitDirtySnapshot(snap);
+	InitDirtySnapshot(snap, NULL);
 	scan = table_beginscan(rel, &snap, 0, NULL);
 	scanslot = table_slot_create(rel, NULL);
 
diff --git a/src/backend/replication/logical/origin.c b/src/backend/replication/logical/origin.c
index 419e4814f0..04ba0d9ba1 100644
--- a/src/backend/replication/logical/origin.c
+++ b/src/backend/replication/logical/origin.c
@@ -278,7 +278,7 @@ replorigin_create(const char *roname)
 	 * to the exclusive lock there's no danger that new rows can appear while
 	 * we're checking.
 	 */
-	InitDirtySnapshot(SnapshotDirty);
+	InitDirtySnapshot(SnapshotDirty, NULL);
 
 	rel = table_open(ReplicationOriginRelationId, ExclusiveLock);
 
diff --git a/src/include/access/transam.h b/src/include/access/transam.h
index 28a2d287fd..aae0ad90c2 100644
--- a/src/include/access/transam.h
+++ b/src/include/access/transam.h
@@ -288,6 +288,7 @@ extern void VarsupShmemInit(void);
 extern FullTransactionId GetNewTransactionId(bool isSubXact);
 extern void AdvanceNextFullTransactionIdPastXid(TransactionId xid);
 extern FullTransactionId ReadNextFullTransactionId(void);
+extern FullTransactionId ReadLastCompletedFullTransactionId(void);
 extern void SetTransactionIdLimit(TransactionId oldest_datfrozenxid,
 								  Oid oldest_datoid);
 extern void AdvanceOldestClogXid(TransactionId oldest_datfrozenxid);
@@ -344,6 +345,21 @@ TransactionIdOlder(TransactionId a, TransactionId b)
 	return b;
 }
 
+/* return the newer of the two IDs */
+static inline TransactionId
+TransactionIdNewer(TransactionId a, TransactionId b)
+{
+	if (!TransactionIdIsValid(a))
+		return b;
+
+	if (!TransactionIdIsValid(b))
+		return a;
+
+	if (TransactionIdPrecedes(a, b))
+		return b;
+	return a;
+}
+
 /* return the older of the two IDs, assuming they're both normal */
 static inline TransactionId
 NormalTransactionIdOlder(TransactionId a, TransactionId b)
diff --git a/src/include/utils/snapmgr.h b/src/include/utils/snapmgr.h
index 9398a84051..5e6f3a7e76 100644
--- a/src/include/utils/snapmgr.h
+++ b/src/include/utils/snapmgr.h
@@ -36,9 +36,13 @@ extern PGDLLIMPORT SnapshotData CatalogSnapshotData;
  * We don't provide a static SnapshotDirty variable because it would be
  * non-reentrant.  Instead, users of that snapshot type should declare a
  * local variable of type SnapshotData, and initialize it with this macro.
+ * pxid is optional and can be NULL. If it is not NULL, pxid[0] will be set
+ * to the transaction ID of deleting transaction if the tuple is deleted
+ * and it newer than pxid[0].
  */
-#define InitDirtySnapshot(snapshotdata)  \
-	((snapshotdata).snapshot_type = SNAPSHOT_DIRTY)
+#define InitDirtySnapshot(snapshotdata, pxid)  \
+	((snapshotdata).snapshot_type = SNAPSHOT_DIRTY, \
+	 (snapshotdata).xip = (pxid))
 
 /*
  * Similarly, some initialization is required for a NonVacuumable snapshot.
diff --git a/src/include/utils/snapshot.h b/src/include/utils/snapshot.h
index 8d1e31e888..a68114e500 100644
--- a/src/include/utils/snapshot.h
+++ b/src/include/utils/snapshot.h
@@ -96,7 +96,10 @@ typedef enum SnapshotType
 	 * xmax.  If the tuple was inserted speculatively, meaning that the
 	 * inserter might still back down on the insertion without aborting the
 	 * whole transaction, the associated token is also returned in
-	 * snapshot->speculativeToken.  See also InitDirtySnapshot().
+	 * snapshot->speculativeToken. If xip is non-NULL, the xid of the
+	 * deleting transaction is stored into xip[0] if it newer than existing
+	 * xip[0] value.
+	 * See also InitDirtySnapshot().
 	 * -------------------------------------------------------------------------
 	 */
 	SNAPSHOT_DIRTY,
diff --git a/src/test/modules/test_misc/t/006_dirty_index_scan.pl b/src/test/modules/test_misc/t/006_dirty_index_scan.pl
new file mode 100644
index 0000000000..4d116e659e
--- /dev/null
+++ b/src/test/modules/test_misc/t/006_dirty_index_scan.pl
@@ -0,0 +1,47 @@
+
+# Copyright (c) 2024, PostgreSQL Global Development Group
+
+# Test issue with lost tuple in case of DirtySnapshot index scans
+use strict;
+use warnings;
+
+use Config;
+use Errno;
+
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+if ($ENV{enable_injection_points} ne 'yes')
+{
+	plan skip_all => 'Injection points not supported by this build';
+}
+
+my ($node, $result);
+$node = PostgreSQL::Test::Cluster->new('DirtyScan_test');
+$node->init;
+$node->append_conf('postgresql.conf', 'fsync = off');
+$node->append_conf('postgresql.conf', 'autovacuum = off');
+$node->start;
+$node->safe_psql('postgres', q(CREATE EXTENSION injection_points));
+$node->safe_psql('postgres', q(CREATE TABLE tbl(i int primary key, n int)));
+
+$node->safe_psql('postgres', q(INSERT INTO tbl VALUES(42,1)));
+$node->safe_psql('postgres', q(SELECT injection_points_attach('check_exclusion_or_unique_constraint_no_conflict', 'error')));
+
+$node->pgbench(
+	'--no-vacuum --client=40 --transactions=1000',
+	0,
+	[qr{actually processed}],
+	[qr{^$}],
+	'concurrent UPSERT',
+	{
+		'on_conflicts' => q(
+			INSERT INTO tbl VALUES(42,1) on conflict(i) do update set n = EXCLUDED.n + 1;
+		)
+	});
+
+$node->safe_psql('postgres', q(SELECT injection_points_detach('check_exclusion_or_unique_constraint_no_conflict')));
+
+$node->stop;
+done_testing();
\ No newline at end of file
-- 
2.34.1

---

From: "Zhijie Hou (Fujitsu)" <[email-masked]>
Date: Thu, 15 Aug 2024 08:06:03 +0000

On Monday, August 12, 2024 7:11 PM Michail Nikolaev <[email-masked]>  wrote:
> Yeah, I think the pgbench-based reproducer may also cause page splits in btree.
> I have attached a reproducer for this case using a spec and injection points.
> 

Thanks a lot for the steps!

I successfully reproduced the issue you mentioned in the context of logical
replication[1]. As you said, it could increase the possibility of tuple missing
when applying updates or deletes in the logical apply worker. I think this is a
long-standing issue and I will investigate the fix you proposed.

In addition, I think the bug is not a blocker for the conflict detection
feature. As the feature simply reports the current behavior of the logical
apply worker (either unique violation or tuple missing) without introducing any
new functionality. Furthermore, I think that the new ExecCheckIndexConstraints
call after ExecInsertIndexTuples() is not affected by the dirty snapshot bug.
This is because a tuple has already been inserted into the btree before the
dirty snapshot scan, which means that a concurrent non-HOT update would not be
possible (it would be blocked after finding the just inserted tuple and wait
for the apply worker to commit the current transaction).

It would be good if others could also share their opinion on this.

[1] The steps to reproduce the tuple missing in logical replication.

1. setup pub/sub env, and publish a table with 1 row.

pub:
CREATE TABLE t(a int primary key, b int);
INSERT INTO t VALUES(1,1);
CREATE PUBLICATION pub FOR TABLE t;

sub:
CREATE TABLE t (a int primary key, b int check (b < 5));
CREATE INDEX t_b_idx ON t(b);
CREATE SUBSCRIPTION sub CONNECTION 'dbname=postgres port=$port_publisher' PUBLICATION pub;

2. Execute an UPDATE(UPDATE t set b = b + 1) on the publisher and use gdb to
stop the apply worker at the point after index_getnext_tid() and before
index_fetch_heap().

3. execute a concurrent update(UPDATE t set b = b + 100) on the subscriber to
update a non-key column value and commit the update.

4. release the apply worker and it would report the update_missing conflict.

Best Regards,
Hou zj

---

From: Michail Nikolaev <[email-masked]>
Date: Fri, 16 Aug 2024 15:31:55 +0200

Hello!

> It would be good if others could also share their opinion on this.

Yes, you are right. At least, I can't find any scenario for that case.

Best regards,
Mikhail.

---

From: Michail Nikolaev <[email-masked]>
Date: Mon, 21 Oct 2024 19:06:59 +0200

Hello, Hou!

I have sent [0] reproducer within the context of conflict detection and
resolution to the original thread.

[0]:
https://www.postgresql.org/message-id/flat/CANtu0ojMjAwMRJK%3DH8y0YBB0ZEcN%2BJbdZeoXQn8dWO5F67jgsA%40mail.gmail.com#f5d1baf4702685aedf23daa9addc012e

---

From: Michail Nikolaev <[email-masked]>
Date: Sat, 23 Nov 2024 17:20:41 +0100

Hello, everyone!

A rebased version is attached. Also, fixed potential race in the test and
detailed commit messages.

Best regards,
Mikhail.

---

From: Michail Nikolaev <[email-masked]>
Date: Mon, 20 Jan 2025 22:35:53 +0100

Hello, everyone!

Simplified (and stabilized, I hope) the test.

Best regards,
Mikhail.

---

From: Michail Nikolaev <[email-masked]>
Date: Wed, 12 Feb 2025 00:39:09 +0100

Hello!

I realize proposed solution does not guarantee absent of false negative
cases...
It happens because I am looking just at XID values, but them have nothing
with transaction commitment order in the common case.

I'll look for some other option.

Best regards,
Mikhail.

---

From: Mihail Nikalayeu <[email-masked]>
Date: Wed, 12 Mar 2025 02:05:50 +0100

Hello, everyone and Peter!

Peter, I have added you because you may be interested in (or already know
about) this btree-related issue.

Short description of the problem:

I noticed a concurrency issue in btree index scans that affects
SnapshotDirty and SnapshotSelf scan types.
When using these non-MVCC snapshot types, a scan could miss tuples if
concurrent transactions delete existing tuples and insert new one with
different TIDs on the same page.

The problem occurs because:
  1. The scan reads a page and caches its tuples in backend-local storage
  2. A concurrent transaction deletes a tuple and inserts a new one with a
different TID
  3. The scan misses the new tuple because it was already deleted by a
committed transaction and does not pass visibility check
  4. But new version on the page is missed, because not in cached tuples

This may cause issues with:
  - logical replication (RelationFindReplTupleByIndex fail) - invalid
conflict message (MISSING instead of ORIGIN_DIFFERS), probably other issues
with upcoming conflict resolution for logical replication
  - check_exclusion_or_unique_constraint false negative (but currently it
does not cause any real issues as far as I can see)

The fix implemented in this version of the patch:
  - Retains the read lock on a page for SnapshotDirty and SnapshotSelf
scans until we're completely done with all tuples from that page
  - Introduces a new 'extra_unlock' field in BTScanPos to track when a lock
is being held longer than usual
  - Updates documentation to explain this special locking behavior

Yes, it may cause some degradation in performance because of that
additional lock.
Another possible idea is to use a fresh MVCC snapshot for such cases (but I
think it is still better to fix or at least document that issue anyway).

Best regards,
Mikhail.

---

From: Mihail Nikalayeu <[email-masked]>
Date: Sun, 18 May 2025 16:36:00 +0200

Hello, everyone!

Rebased + fix for compilation due the new INEJCTION_POINT signature.

Best regards,
Mikhail.

---

From: Mihail Nikalayeu <[email-masked]>
Date: Tue, 10 Jun 2025 08:00:00 +0200

Hello!

Rebased\reworked to align with the changes of [0].

Best regards.

[0]: https://www.postgresql.org/message-id/flat/CAH2-WznZBhWqDBDVGh1VhVBLgLqaYHEkPhmVV7mJCr1Y3ZQhQQ%40mail.gmail.com#d6f1debb1405d1b4a983cbb46b24f41b

---

From: Mihail Nikalayeu <[email-masked]>
Date: Mon, 16 Jun 2025 22:25:23 +0200

Rebased.

---

From: Mihail Nikalayeu <[email-masked]>
Date: Wed, 20 Aug 2025 00:44:00 +0200

Hello, everyone!

Issue description is available at [0] (in few words - SnapshotDirty
scan may miss tuple in index because of race condition with update of
that tuple).

But I have realised there are cases much more severe than invalid
conflict messages for logical replication - lost delete/updates in
logical replication.
New tests with reproducers are included in the new patch version.

Short description up issues:

1) Lost delete

Setup:
  CREATE TABLE conf_tab(a int PRIMARY key, data text);
  CREATE INDEX data_index ON conf_tab(data);
  INSERT INTO conf_tab(a, data) VALUES (1,'frompub');

On publisher:
  DELETE FROM conf_tab WHERE a=1;

On subscriber:
  UPDATE conf_tab SET data = 'fromsubnew' WHERE (a=1);

Expected result:
  Tuple is deleted on both subscriber and publisher.

Actual result:
  Either as expected, or:
  - Tuple is deleted on publisher, but 'fromsubnew' remains on subscriber.

2) Lost update

Setup:

On publisher:
  CREATE TABLE conf_tab(a int PRIMARY key, data text);
  INSERT INTO conf_tab(a, data) VALUES (1,'frompub');

On subscriber:
  -- note additional subscriber-only column - i
  CREATE TABLE conf_tab(a int PRIMARY key, data text, i int DEFAULT 0);
  CREATE INDEX i_index ON conf_tab(i);

On publisher:
  UPDATE conf_tab SET data = 'frompubnew' WHERE (a=1);

On subscriber:
  UPDATE conf_tab SET i = 1 WHERE (a=1);

Expected result:
  On subscriber: tuple (a=1, data='frompubnew', i=1).

Actual result:
  Either as expected, or:
  - Publisher update is lost, leaving (a=1, data='frompub', i=1) on subscriber.

Best regards,
Mikhail.

[0]: https://www.postgresql.org/message-id/flat/CADzfLwWuXh8KO%3DOZvB71pZnQ8nH0NYXfuGbFU6FBiVZUbmuFGg%40mail.gmail.com#76f98a9ae3479bbaf5ee9262322d466e

---

From: Mihail Nikalayeu <[email-masked]>
Date: Thu, 21 Aug 2025 10:43:08 +0200

Hello,

Added one more test - for invalid "update_deleted" conflict detection.

Best regards,
Mikhail.

---

From: Amit Kapila <[email-masked]>
Date: Fri, 22 Aug 2025 16:20:41 +0530

On Wed, Mar 12, 2025 at 6:36 AM Mihail Nikalayeu
<[email-masked]> wrote:
> Peter, I have added you because you may be interested in (or already know about) this btree-related issue.
>
> I noticed a concurrency issue in btree index scans that affects SnapshotDirty and SnapshotSelf scan types.
> When using these non-MVCC snapshot types, a scan could miss tuples if concurrent transactions delete existing tuples and insert new one with different TIDs on the same page.
>
>   2. A concurrent transaction deletes a tuple and inserts a new one with a different TID
>   3. The scan misses the new tuple because it was already deleted by a committed transaction and does not pass visibility check

IIUC, the problem you are worried about can happen with DELETE+INSERT
in the same transaction on the subscriber, right? If so, this should
happen with DELETE and INSERT in a separate transaction as well. If
that happens then we anyway may not be able to detect such an INSERT
if it happens on a page earlier than the current page.

BTW, as the update (or DELETE+INSERT) happens at a later time than the
publisher's update/delete, so once we have the last_write_win
resolution strategy implemented, it is the subscriber operation that
will win. So, the current behavior shouldn't cause any problem.

-- 
With Regards,
Amit Kapila.

---

From: Mihail Nikalayeu <[email-masked]>
Date: Fri, 22 Aug 2025 14:47:57 +0200

Hello, Amit,

> IIUC, the problem you are worried about can happen with DELETE+INSERT
It seems there was some misunderstanding due to my bad explanation and wording.
I wrote "A concurrent transaction deletes a tuple and inserts a new
one with a different TID" - but I mean logical UPDATE causing new TID
in index page appear because HOT was applied...

Lets try again, I hope that explanation is better:

At the start, we have a table with a primary key and one extra index
(to disable HOT), and a tuple with i=13:

CREATE TABLE table (i int PRIMARY KEY, data text);
CREATE INDEX no_more_hot_data_index ON table (data);
INSERT INTO table (i, data) VALUES (13, 'data');

A btree scan using SnapshotDirty can miss tuples because of internal
locking logic. Here’s how the bug shows up:

1) we have a tuple in the index (i=13), committed long ago
2) transaction A starts an index search for that tuple using
SnapshotDirty (WHERE i = 13)
3) in parallel, transaction B updates that tuple (SET data='updated'
WHERE i=13) and commits (creating a new index entry because HOT is not
applied)
4) the scan from step 2 returns nothing at all - as if the tuple never existed

In other words, if you start a SnapshotDirty btree scan for i=13 and
update that row i=13 at the same physical moment, the scan may:
* return the TID of the pre‑update version - correct behavior
* return the TID of the post‑update version - also correct
* return nothing - this is the broken case

More broadly: any SnapshotDirty scan may completely miss existing data
when there are concurrent updates.

SnapshotDirty usage in Postgres is limited, so the impact isn’t huge,
but every case I found is reproducible with the tests from the first
commit from v10 in my previous email.

* check_exclusion_or_unique_constraint: only a minor performance
impact, handled by retry logic
* logical replication TAP tests: multiple scenarios fail because
RelationFindReplTupleByIndex cannot find existing committed tuples

These scenarios look like:

1) logical replication tries to apply a change for tuple X received
from the publisher
2) meanwhile, the subscriber updates the same tuple X and commits in
parallel transaction
3) due to the bug, RelationFindReplTupleByIndex concludes the tuple X
does not exist at all, leading to bad outcomes, including:
     * incorrect conflict‑type messages (and, in the future,
potentially wrong conflict‑resolution choices)
     * lost updates (see scenario 2 from [0])

If you look at the tests and play with the $simulate_race_condition
flag, you can see the behavior directly. The second commit (a possible
fix) in v10 also includes documentation updates that try to explain
the issue in a more appropriate context.

I’m happy to provide additional reproducers or explanations if that would help.

[0]: https://www.postgresql.org/message-id/flat/CADzfLwWC49oanFSGPTf%3D6FJoTw-kAnpPZV8nVqAyR5KL68LrHQ%40mail.gmail.com#5f6b3be849f8d95c166decfae541df09

Best regards,
Mikhail.

---

From: Mihail Nikalayeu <[email-masked]>
Date: Fri, 22 Aug 2025 14:51:45 +0200

Oh,

> in index page appear because HOT was applied...

I mean "HOT was NOT applied", sorry for the inconvenience.

---

From: Mihail Nikalayeu <[email-masked]>
Date: Fri, 22 Aug 2025 17:41:25 +0200

Amit, a few more explanations related to your message.

> in the same transaction on the subscriber, right?

Technically, yes - this can occur during a single UPDATE, as well as a
DELETE followed by an INSERT of the same key within the same
transaction (which is effectively equivalent to an UPDATE). However,
it should NOT occur, because at no point in the timeline does a row
with that key fail to exist; therefore, no scan should return “there
is no such row in the index.”

> If so, this should
> happen with DELETE and INSERT in a separate transaction as well.

Yes, it may happen - and in that case, it is correct. This is because
there is a moment between the DELETE and the INSERT when the row does
not exist. Therefore, it is acceptable for a scan to check the index
at that particular moment and find nothing.

> will win. So, the current behavior shouldn't cause any problem.

For the last_write_win and UPDATE vs UPDATE case - yes, probably, but
only by luck.

However, there are many scenarios that cannot be implemented
correctly, for example:
* DELETE always wins
* UPDATE with a higher version (column value) wins
* first_write_win
* etc.

Also, the cases from [0] are clearly wrong without any conflict
resolution. In particular, case 2 - there are no real conflicts at all
(since different sets of columns are involved), but an incorrect
result may still be produced.

---

From: Amit Kapila <[email-masked]>
Date: Mon, 25 Aug 2025 10:31:17 +0530

> For the last_write_win and UPDATE vs UPDATE case - yes, probably, but

Why only by luck?

> * UPDATE with a higher version (column value) wins

Then these may not lead to eventual consistency for such cases. So,
not sure one should anyway rely on these.

> result may still be produced.

I think this questions whether we consider the SnapshotDirty results
correct or not. The case of logical replication giving wrong results
[0] is the behavior from the beginning of logical replication. Now, I
would like to know the opinion of others who were involved in the
initial commit, so added Peter E. to see what he thinks of the same.
If we don't get the opinion here (say people missed to read because of
an unrelated title) then I suggest you start a separate email thread
to discuss just that case and see what others think.

-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Mon, 25 Aug 2025 10:32:12 +0530

On Fri, Aug 22, 2025 at 9:12 PM Mihail Nikalayeu
<[email-masked]> wrote:
> transaction (which is effectively equivalent to an UPDATE).
>

BTW, then isn't it possible that INSERT happens on a different page?

-- 
With Regards,
Amit Kapila.

---

From: Mihail Nikalayeu <[email-masked]>
Date: Mon, 25 Aug 2025 12:49:10 +0200

Hello!

> Why only by luck?

I mean last_write_win provides the same results in the following cases:
* we found the tuple, detected a conflict, and decided to ignore the
update coming from the publisher
* we were unable to find the tuple, logged an error about it, and
ignored the update coming from the publisher

In both cases, the result is the same: the subscriber version remains
in the table.

> not sure one should anyway rely on these.

But with the fixed snapshot dirty scan, it becomes possible to
implement such strategies.
Also, some strategies require some kind of merge function for tuples.
In my understanding, even last_write_win should probably compare
timestamps to determine which version is "newer" because time in
distributed systems can be tricky.
Therefore, we have to find the tuple if it exists.

> BTW, then isn't it possible that INSERT happens on a different page?

Yes, it is possible - in that case, the bug does not occur. It only
happens if a new TID of some logical tuple is added to the same page.
Just to clarify, this is about B-tree pages, not the heap.

> correct or not.

In my understanding, this is clearly wrong:
* such behavior is not documented anywhere
* usage patterns assume that such things cannot happen
* new features struggle with it. For example, the new update_deleted
logging may fail to behave correctly
(038_update_missing_with_retain.pl in the patch) - so how should it be
used? It might be correct, but it also might not be...

Another option is to document the behavior and rename it to SnapshotMaybe :)
By the way, SnapshotSelf is also affected.

> The case of logical replication giving wrong results
> [0] is the behavior from the beginning of logical replication.

Logical replication was mainly focused on replication without any
concurrent updates on the subscriber side. So, I think this is why the
issue was overlooked.

Best regards,
Mikhail.

---

From: Amit Kapila <[email-masked]>
Date: Mon, 25 Aug 2025 17:21:27 +0530

> In both cases, the result is the same: the subscriber version remains

Right, so we can say that it will be consistent.

> happens if a new TID of some logical tuple is added to the same page.

What if the new insert happens in a page prior to the current page? I
mean that the scan won't encounter the page where Insert happens.

> * such behavior is not documented anywhere

I agree. This is where we need inputs.

> By the way, SnapshotSelf is also affected.

BTW, do we know the reason behind using SnapshotDirty in the first
place? I don't see any comments in the nearby code unless I am missing
something.

> concurrent updates on the subscriber side. So, I think this is why the

The other possibility is that as this is a rare scenario so we didn't
consider it.

-- 
With Regards,
Amit Kapila.

---

From: Mihail Nikalayeu <[email-masked]>
Date: Mon, 25 Aug 2025 15:32:03 +0200

> mean that the scan won't encounter the page where Insert happens.

Hmm.... Yes - if the TID lands to the page left of the current
position, we’ll miss it as well.
A lock‑based solution (version in the v10) would require keeping all
pages with the same key under a read lock, which feels too expensive.

> place? I don't see any comments in the nearby code unless I am missing

I think this is simply an attempt to lock the newest version of the
logical tuple, including INSERT cases.
For an existing tuple, the same can be achieved using MVCC snapshot + retry.
However, in the case of a not-yet-committed INSERT, a different type
of snapshot is required.

But I'm not sure if it provides any advantages.

---

From: Amit Kapila <[email-masked]>
Date: Tue, 26 Aug 2025 16:38:17 +0530

> pages with the same key under a read lock, which feels too expensive.

Right.

> But I'm not sure if it provides any advantages.

I think it is better to document this race somewhere in a logical
replication document for now unless we have a consensus on a way to
move forward.

-- 
With Regards,
Amit Kapila.

---

From: Mihail Nikalayeu <[email-masked]>
Date: Wed, 27 Aug 2025 10:53:57 +0200

Hello, Amit!

Amit Kapila <[email-masked]>:

> Now, I

Seems like you added another Peter in [0] - I added Peter Eisentraut :)

> > pages with the same key under a read lock, which feels too expensive.

I think it is possible to achieve the same guarantees and logic using
GetLatestSnapshot + HeapTupleSatisfiesDirty, but without the "tuple
not found" case - I'll try to experiment with it.
GetLatestSnapshot is called before tuple lock anyway.

> replication document for now unless we have a consensus on a way to

Yes, it is an option, but what documentation is going to be strange:

* there is delete_missing type of conflict stats\logs, but be aware it
may be wrong (actually it delete_missing)
* the same for update_missing vs update_origin_differs
* the same for update_deleted vs update_origin_differs
* also DELETE or UPDATE from publisher may be missed in case of update
on subscriber even if update touches subscriber-only columns

It looks like "if something is updating on subscriber - no
guarantees". And the worst thing - it is the actual state.

[0]: https://www.postgresql.org/message-id/flat/CAA4eK1LZxzORgAoDhix9MWrOqYOsNZuZLW2sTfGsJFM99yRgrg%40mail.gmail.com#02be86f7e2d24a038878f03ac1b93e95

Best regards,
MIkhail.

---

From: Mihail Nikalayeu <[email-masked]>
Date: Wed, 3 Sep 2025 20:20:21 +0200

Hello, Amit!

Peter answered in [0]:
> I don’t remember. I was just the committer.

I’ve attached a new version of the proposed solution.
The first commit includes tests, some README updates, and an
additional pgbench test that reproduces the issue without explicitly
simulating the wait/resume race. This last test is heavy and isn't
intended to be committed.

Instead of adding extra locking in btree, a more lightweight approach
is used: since we already call GetLatestSnapshot before
table_tuple_lock, we can simply call it before each scan attempt and
use that snapshot for the scan.

As a result:
* MVCC scan will not miss updated tuples, while DirtyScan may
* in both cases, table_tuple_lock will wait for the updating
transaction to commit before retrying
* MVCC scan cannot see not-yet-committed new rows, while DirtyScan
can. However, this does not provide any stronger guarantee: in the
case of INSERT vs INSERT, two parallel inserts are still possible.
DirtyScan only slightly reduces the probability, but if the scan does
not find the row, there is still no guarantee that it won’t be
inserted immediately afterward.

Therefore, the MVCC version appears to provide the same guarantees,
without missing tuples, and with the same performance.

Best regards,
Mikhail.

[0]:
https://discord.com/channels/1258108670710124574/1407753138991009913/1411303541900841090

---

From: Mihail Nikalayeu <[email-masked]>
Date: Sat, 13 Sep 2025 14:07:33 +0200

Rebased.

Also, separate thread with some additional explanation is here:

https://www.postgresql.org/message-id/flat/[email-masked]

---

From: Mihail Nikalayeu <[email-masked]>
Date: Sat, 25 Oct 2025 16:55:52 +0200

Rebased again.

---

From: Mihail Nikalayeu <[email-masked]>
Date: Tue, 28 Oct 2025 19:06:00 +0100

Fixed race in tests caused
https://cirrus-ci.com/task/5815107659235328?logs=test_world#L324 to
fail.
From 2a6a121c7cfe4823db0f8aec931c5dbcab672616 Mon Sep 17 00:00:00 2001
From: nkey <[email-masked]>
Date: Wed, 3 Sep 2025 19:08:55 +0200
Subject: [PATCH v14 2/2] Fix logical replication conflict detection during
 tuple lookup

SNAPSHOT_DIRTY scans could miss conflict detection with concurrent transactions during logical replication.
Replace SNAPSHOT_DIRTY scan with the GetLatestSnapshot in RelationFindReplTupleByIndex and RelationFindReplTupleSeq.
---
 src/backend/executor/execReplication.c | 63 ++++++++------------------
 1 file changed, 18 insertions(+), 45 deletions(-)

diff --git a/src/backend/executor/execReplication.c b/src/backend/executor/execReplication.c
index b409d4ecbf5..0de40aec733 100644
--- a/src/backend/executor/execReplication.c
+++ b/src/backend/executor/execReplication.c
@@ -186,8 +186,6 @@ RelationFindReplTupleByIndex(Relation rel, Oid idxoid,
 	ScanKeyData skey[INDEX_MAX_KEYS];
 	int			skey_attoff;
 	IndexScanDesc scan;
-	SnapshotData snap;
-	TransactionId xwait;
 	Relation	idxrel;
 	bool		found;
 	TypeCacheEntry **eq = NULL;
@@ -198,17 +196,17 @@ RelationFindReplTupleByIndex(Relation rel, Oid idxoid,

-
 
-	/* Start an index scan. */
-	scan = index_beginscan(rel, idxrel, &snap, NULL, skey_attoff, 0);
+	/* Start an index scan. SnapshotAny will be replaced below. */
+	scan = index_beginscan(rel, idxrel, SnapshotAny, NULL, skey_attoff, 0);
 
 retry:
 	found = false;
-
+	PushActiveSnapshot(GetLatestSnapshot());
+	/* Update the actual scan snapshot each retry */
+	scan->xs_snapshot = GetActiveSnapshot();
 	index_rescan(scan, skey, skey_attoff, NULL, 0);
 
 	/* Try to find the tuple */
@@ -229,19 +227,6 @@ retry:
 
 		ExecMaterializeSlot(outslot);
 
-		xwait = TransactionIdIsValid(snap.xmin) ?
-			snap.xmin : snap.xmax;
-
-		/*
-		 * If the tuple is locked, wait for locking transaction to finish and
-		 * retry.
-		 */
-		if (TransactionIdIsValid(xwait))
-		{
-			XactLockTableWait(xwait, NULL, NULL, XLTW_None);
-			goto retry;
-		}
-
 		/* Found our tuple and it's not locked */
 		found = true;
 		break;
@@ -253,8 +238,6 @@ retry:
 		TM_FailureData tmfd;
 		TM_Result	res;
 
-		PushActiveSnapshot(GetLatestSnapshot());
-
 		res = table_tuple_lock(rel, &(outslot->tts_tid), GetActiveSnapshot(),
 							   outslot,
 							   GetCurrentCommandId(false),
@@ -263,13 +246,15 @@ retry:
 							   0 /* don't follow updates */ ,
 							   &tmfd);
 
-		PopActiveSnapshot();
-
 		if (should_refetch_tuple(res, &tmfd))
+		{
+			PopActiveSnapshot();
 			goto retry;
+		}
 	}
 
 	index_endscan(scan);
+	PopActiveSnapshot();
 
 	/* Don't release lock until commit. */
 	index_close(idxrel, NoLock);
@@ -370,9 +355,7 @@ RelationFindReplTupleSeq(Relation rel, LockTupleMode lockmode,
 {
 	TupleTableSlot *scanslot;
 	TableScanDesc scan;
-	SnapshotData snap;
 	TypeCacheEntry **eq;
-	TransactionId xwait;
 	bool		found;
 	TupleDesc	desc PG_USED_FOR_ASSERTS_ONLY = RelationGetDescr(rel);
 
@@ -380,13 +363,15 @@ RelationFindReplTupleSeq(Relation rel, LockTupleMode lockmode,

-	/* Start a heap scan. */
-	InitDirtySnapshot(snap);
-	scan = table_beginscan(rel, &snap, 0, NULL);
+	/* Start a heap scan. SnapshotAny will be replaced below. */
+	scan = table_beginscan(rel, SnapshotAny, 0, NULL);
 	scanslot = table_slot_create(rel, NULL);
 
 retry:
 	found = false;
+	PushActiveSnapshot(GetLatestSnapshot());
+	/* Update the actual scan snapshot each retry */
+	scan->rs_snapshot = GetActiveSnapshot();
 
 	table_rescan(scan, NULL);
 
@@ -399,19 +384,6 @@ retry:
 		found = true;
 		ExecCopySlot(outslot, scanslot);
 
-		xwait = TransactionIdIsValid(snap.xmin) ?
-			snap.xmin : snap.xmax;
-
-		/*
-		 * If the tuple is locked, wait for locking transaction to finish and
-		 * retry.
-		 */
-		if (TransactionIdIsValid(xwait))
-		{
-			XactLockTableWait(xwait, NULL, NULL, XLTW_None);
-			goto retry;
-		}
-
 		/* Found our tuple and it's not locked */
 		break;
 	}
@@ -422,8 +394,6 @@ retry:
 		TM_FailureData tmfd;
 		TM_Result	res;
 
-		PushActiveSnapshot(GetLatestSnapshot());
-
 		res = table_tuple_lock(rel, &(outslot->tts_tid), GetActiveSnapshot(),
 							   outslot,
 							   GetCurrentCommandId(false),
@@ -432,13 +402,16 @@ retry:
 							   0 /* don't follow updates */ ,
 							   &tmfd);
 
-		PopActiveSnapshot();
 
 		if (should_refetch_tuple(res, &tmfd))
+		{
+			PopActiveSnapshot();
 			goto retry;
+		}
 	}
 
 	table_endscan(scan);
+	PopActiveSnapshot();
 	ExecDropSingleTupleTableSlot(scanslot);
 
 	return found;
-- 
2.48.1

From 66ede0f9001d9e841e9249091869dbde58df4d5c Mon Sep 17 00:00:00 2001
From: nkey <[email-masked]>
Date: Sat, 23 Nov 2024 13:25:11 +0100
Subject: [PATCH v14 1/2] This patch introduces new injection points and TAP
 tests to reproduce and verify conflict detection issues that arise during
 SNAPSHOT_DIRTY index scans in logical replication.

---
 src/backend/access/index/indexam.c            |   9 ++
 src/backend/access/nbtree/README              |   9 ++
 src/backend/executor/execIndexing.c           |   7 +-
 src/backend/replication/logical/worker.c      |   4 +
 src/include/utils/snapshot.h                  |  14 ++
 src/test/subscription/meson.build             |   4 +
 .../subscription/t/036_delete_missing_race.pl | 139 +++++++++++++++++
 .../subscription/t/037_update_missing_race.pl | 141 +++++++++++++++++
 .../t/038_update_missing_with_retain.pl       | 143 ++++++++++++++++++
 .../t/039_update_missing_simulation.pl        | 125 +++++++++++++++
 10 files changed, 594 insertions(+), 1 deletion(-)
 create mode 100644 src/test/subscription/t/036_delete_missing_race.pl
 create mode 100644 src/test/subscription/t/037_update_missing_race.pl
 create mode 100644 src/test/subscription/t/038_update_missing_with_retain.pl
 create mode 100644 src/test/subscription/t/039_update_missing_simulation.pl

diff --git a/src/backend/access/index/indexam.c b/src/backend/access/index/indexam.c
index 86d11f4ec79..a503fa02ac5 100644
--- a/src/backend/access/index/indexam.c
+++ b/src/backend/access/index/indexam.c
@@ -52,11 +52,13 @@
 #include "catalog/pg_type.h"
 #include "nodes/execnodes.h"
 #include "pgstat.h"
+#include "replication/logicalworker.h"
 #include "storage/lmgr.h"
 #include "storage/predicate.h"
 #include "utils/ruleutils.h"
 #include "utils/snapmgr.h"
 #include "utils/syscache.h"
+#include "utils/injection_point.h"

 /* ----------------------------------------------------------------
@@ -751,6 +753,13 @@ index_getnext_slot(IndexScanDesc scan, ScanDirection direction, TupleTableSlot *
 		 * the index.
 		 */
 		Assert(ItemPointerIsValid(&scan->xs_heaptid));
+#ifdef USE_INJECTION_POINTS
+		if (!IsCatalogRelation(scan->heapRelation) && IsLogicalWorker())
+		{
+			INJECTION_POINT("index_getnext_slot_before_fetch_apply_dirty", NULL);
+		}
+#endif
+
 		if (index_fetch_heap(scan, slot))
 			return true;
 	}
diff --git a/src/backend/access/nbtree/README b/src/backend/access/nbtree/README
index 53d4a61dc3f..634a3d10bb1 100644
--- a/src/backend/access/nbtree/README
+++ b/src/backend/access/nbtree/README
@@ -103,6 +103,15 @@ We also remember the left-link, and follow it when the scan moves backwards
 (though this requires extra handling to account for concurrent splits of
 the left sibling; see detailed move-left algorithm below).
 
+Despite the described mechanics in place, inconsistent results may still occur
+during non-MVCC scans (SnapshotDirty and SnapshotSelf). This issue can occur if a 
+concurrent transaction deletes a tuple and inserts a new tuple with a new TID in the 
+same page or to the left/right (depending on scan direction) of current scan position.
+If the scan has already visited the page and cached its content in the
+backend-local storage, it might skip the old tuple due to deletion and miss the new 
+tuple because the scan does not re-read the page. Note it affects not only btree
+scan but also a heap scan.
+
 In most cases we release our lock and pin on a page before attempting
 to acquire pin and lock on the page we are moving to.  In a few places
 it is necessary to lock the next page before releasing the current one.
diff --git a/src/backend/executor/execIndexing.c b/src/backend/executor/execIndexing.c
index ca33a854278..61a5097f789 100644
--- a/src/backend/executor/execIndexing.c
+++ b/src/backend/executor/execIndexing.c
@@ -117,6 +117,7 @@
 #include "utils/multirangetypes.h"
 #include "utils/rangetypes.h"
 #include "utils/snapmgr.h"
+#include "utils/injection_point.h"
 
 /* waitMode argument to check_exclusion_or_unique_constraint() */
 typedef enum
@@ -780,7 +781,9 @@ check_exclusion_or_unique_constraint(Relation heap, Relation index,
 	/*
 	 * Search the tuples that are in the index for any violations, including
 	 * tuples that aren't visible yet.
-	 */
+	 * Snapshot dirty may miss some tuples in the case of parallel updates,
+	 * but it is acceptable here.
+	*/
 	InitDirtySnapshot(DirtySnapshot);
 
 	for (i = 0; i < indnkeyatts; i++)
@@ -943,6 +946,8 @@ retry:

+	if (!conflict)
+		INJECTION_POINT("check_exclusion_or_unique_constraint_no_conflict", NULL);
 	return !conflict;
 }
 
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index ee6ac22329f..cccbaeedfd7 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -277,6 +277,7 @@
 #include "tcop/tcopprot.h"
 #include "utils/acl.h"
 #include "utils/guc.h"
+#include "utils/injection_point.h"
 #include "utils/inval.h"
 #include "utils/lsyscache.h"
 #include "utils/memutils.h"
@@ -2946,7 +2947,10 @@ apply_handle_update_internal(ApplyExecutionData *edata,
 			conflicttuple.origin != replorigin_session_origin)
 			type = CT_UPDATE_DELETED;
 		else
+		{
+			INJECTION_POINT("apply_handle_update_internal_update_missing", NULL);
 			type = CT_UPDATE_MISSING;
+		}
 
 		/* Store the new tuple for conflict reporting */
 		slot_store_data(newslot, relmapentry, newtup);
diff --git a/src/include/utils/snapshot.h b/src/include/utils/snapshot.h
index 0e546ec1497..189dfd71103 100644
--- a/src/include/utils/snapshot.h
+++ b/src/include/utils/snapshot.h
@@ -53,6 +53,13 @@ typedef enum SnapshotType
 	 * - previous commands of this transaction
 	 * - changes made by the current command
 	 *
+	 * Note: such a snapshot may miss an existing logical tuple in case of
+	 * parallel update.
+	 * If a new version of a tuple is inserted into an already processed page
+	 * but the old one marked with committed xmax - snapshot will skip the old
+	 * one and never meet the new one during that scan - resulting in skipping
+	 * that tuple at all.
+	 *
 	 * Does _not_ include:
 	 * - in-progress transactions (as of the current instant)
 	 * -------------------------------------------------------------------------
@@ -82,6 +89,13 @@ typedef enum SnapshotType
 	 * transaction and committed/aborted xacts are concerned.  However, it
 	 * also includes the effects of other xacts still in progress.
 	 *
+	 * Note: such a snapshot may miss an existing logical tuple in case of
+	 * parallel update.
+	 * If a new version of a tuple is inserted into an already processed page but the
+	 * old one marked with committed/in-progress xmax - snapshot will skip the old one
+	 * and never meet the new one during that scan - resulting in skipping that tuple
+	 * at all.
+	 *
 	 * A special hack is that when a snapshot of this type is used to
 	 * determine tuple visibility, the passed-in snapshot struct is used as an
 	 * output argument to return the xids of concurrent xacts that affected
diff --git a/src/test/subscription/meson.build b/src/test/subscription/meson.build
index 20b4e523d93..4f9a5c9209d 100644
--- a/src/test/subscription/meson.build
+++ b/src/test/subscription/meson.build
@@ -45,6 +45,10 @@ tests += {
       't/033_run_as_table_owner.pl',
       't/034_temporal.pl',
       't/035_conflicts.pl',
+      't/036_delete_missing_race.pl',
+      't/037_update_missing_race.pl',
+      't/038_update_missing_with_retain.pl',
+      't/039_update_missing_simulation.pl',
       't/100_bugs.pl',
     ],
   },
diff --git a/src/test/subscription/t/036_delete_missing_race.pl b/src/test/subscription/t/036_delete_missing_race.pl
new file mode 100644
index 00000000000..51dd351dc10
--- /dev/null
+++ b/src/test/subscription/t/036_delete_missing_race.pl
@@ -0,0 +1,139 @@
+# Copyright (c) 2025, PostgreSQL Global Development Group
+
+# Test the conflict detection and resolution in logical replication
+use strict;
+use warnings FATAL => 'all';
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+if ($ENV{enable_injection_points} ne 'yes')
+{
+	plan skip_all => 'Injection points not supported by this build';
+}
+
+############################## Set it to 0 to make set success; TODO: delete that for commit
+my $simulate_race_condition = 1;
+##############################
+
+###############################
+# Setup
+###############################
+
+# Initialize publisher node
+my $node_publisher = PostgreSQL::Test::Cluster->new('publisher');
+$node_publisher->init(allows_streaming => 'logical');
+$node_publisher->append_conf('postgresql.conf',
+	qq(track_commit_timestamp = on));
+$node_publisher->start;
+
+
+# Create subscriber node with track_commit_timestamp enabled
+my $node_subscriber = PostgreSQL::Test::Cluster->new('subscriber');
+$node_subscriber->init;
+$node_subscriber->append_conf('postgresql.conf',
+	qq(track_commit_timestamp = on));
+$node_subscriber->start;
+
+
+# Check if the extension injection_points is available, as it may be
+# possible that this script is run with installcheck, where the module
+# would not be installed by default.
+if (!$node_subscriber->check_extension('injection_points'))
+{
+	plan skip_all => 'Extension injection_points not installed';
+}
+
+# Create table on publisher
+$node_publisher->safe_psql(
+	'postgres',
+	"CREATE TABLE conf_tab(a int PRIMARY key, data text);");
+
+# Create similar table on subscriber with additional index to disable HOT updates
+$node_subscriber->safe_psql(
+	'postgres',
+	"CREATE TABLE conf_tab(a int PRIMARY key, data text);
+	 CREATE INDEX data_index ON conf_tab(data);");
+
+# Set up extension to simulate race condition
+$node_subscriber->safe_psql('postgres', 'CREATE EXTENSION injection_points;');
+
+# Setup logical replication
+my $publisher_connstr = $node_publisher->connstr . ' dbname=postgres';
+$node_publisher->safe_psql('postgres',
+	"CREATE PUBLICATION tap_pub FOR TABLE conf_tab");
+
+# Insert row to be updated later
+$node_publisher->safe_psql('postgres',
+	"INSERT INTO conf_tab(a, data) VALUES (1,'frompub')");
+
+# Create the subscription
+my $appname = 'tap_sub';
+$node_subscriber->safe_psql(
+	'postgres',
+	"CREATE SUBSCRIPTION tap_sub
+	 CONNECTION '$publisher_connstr application_name=$appname'
+	 PUBLICATION tap_pub");
+
+# Wait for initial table sync to finish
+$node_subscriber->wait_for_subscription_sync($node_publisher, $appname);
+
+############################################
+# Race condition because of DirtySnapshot
+############################################
+
+my $psql_session_subscriber = $node_subscriber->background_psql('postgres');
+if ($simulate_race_condition)
+{
+	$node_subscriber->safe_psql('postgres',
+		"SELECT injection_points_attach('index_getnext_slot_before_fetch_apply_dirty', 'wait')");
+}
+
+my $log_offset = -s $node_subscriber->logfile;
+
+# Delete tuple on publisher
+$node_publisher->safe_psql('postgres', "DELETE FROM conf_tab WHERE a=1;");
+
+if ($simulate_race_condition)
+{
+	# Wait apply worker to start the search for the tuple using index
+	$node_subscriber->wait_for_event('logical replication apply worker',
+		'index_getnext_slot_before_fetch_apply_dirty');
+}
+
+# Updater tuple on subscriber
+$psql_session_subscriber->query_until(
+	qr/start/, qq[
+	\\echo start
+	UPDATE conf_tab SET data = 'fromsubnew' WHERE (a=1);
+]);
+
+
+if ($simulate_race_condition)
+{
+	# Wake up apply worker
+	$node_subscriber->safe_psql('postgres',"
+		SELECT injection_points_detach('index_getnext_slot_before_fetch_apply_dirty');
+		SELECT injection_points_wakeup('index_getnext_slot_before_fetch_apply_dirty');
+		");
+}
+
+# Tuple was updated - so, we have conflict
+$node_subscriber->wait_for_log(
+	qr/conflict detected on relation \"public.conf_tab\"/,
+	$log_offset);
+
+$node_publisher->wait_for_catchup($appname);
+
+# But tuple should be deleted on subscriber any way
+is($node_subscriber->safe_psql('postgres', 'SELECT count(*) from conf_tab'), 0, 'record deleted on subscriber');
+
+ok(!$node_subscriber->log_contains(
+		qr/LOG:  conflict detected on relation \"public.conf_tab\": conflict=delete_missing/,
+		$log_offset), 'invalid conflict detected');
+
+ok($node_subscriber->log_contains(
+		qr/LOG:  conflict detected on relation \"public.conf_tab\": conflict=delete_origin_differs/,
+		$log_offset), 'correct conflict detected');
+
+done_testing();
diff --git a/src/test/subscription/t/037_update_missing_race.pl b/src/test/subscription/t/037_update_missing_race.pl
new file mode 100644
index 00000000000..1e120f74bbd
--- /dev/null
+++ b/src/test/subscription/t/037_update_missing_race.pl
@@ -0,0 +1,141 @@
+# Copyright (c) 2025, PostgreSQL Global Development Group
+
+# Test the conflict detection and resolution in logical replication
+use strict;
+use warnings FATAL => 'all';
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+if ($ENV{enable_injection_points} ne 'yes')
+{
+	plan skip_all => 'Injection points not supported by this build';
+}
+
+############################## Set it to 0 to make set success; TODO: delete that for commit
+my $simulate_race_condition = 1;
+##############################
+
+###############################
+# Setup
+###############################
+
+# Initialize publisher node
+my $node_publisher = PostgreSQL::Test::Cluster->new('publisher');
+$node_publisher->init(allows_streaming => 'logical');
+$node_publisher->append_conf('postgresql.conf',
+	qq(track_commit_timestamp = on));
+$node_publisher->start;
+
+
+# Create subscriber node with track_commit_timestamp enabled
+my $node_subscriber = PostgreSQL::Test::Cluster->new('subscriber');
+$node_subscriber->init;
+$node_subscriber->append_conf('postgresql.conf',
+	qq(track_commit_timestamp = on));
+$node_subscriber->start;
+
+
+# Check if the extension injection_points is available, as it may be
+# possible that this script is run with installcheck, where the module
+# would not be installed by default.
+if (!$node_subscriber->check_extension('injection_points'))
+{
+	plan skip_all => 'Extension injection_points not installed';
+}
+
+# Create table on publisher
+$node_publisher->safe_psql(
+	'postgres',
+	"CREATE TABLE conf_tab(a int PRIMARY key, data text);");
+
+# Create similar table on subscriber with additional index to disable HOT updates and additional column
+$node_subscriber->safe_psql(
+	'postgres',
+	"CREATE TABLE conf_tab(a int PRIMARY key, data text, i int DEFAULT 0);
+	 CREATE INDEX i_index ON conf_tab(i);");
+
+# Set up extension to simulate race condition
+$node_subscriber->safe_psql('postgres', 'CREATE EXTENSION injection_points;');
+
+# Setup logical replication
+my $publisher_connstr = $node_publisher->connstr . ' dbname=postgres';
+$node_publisher->safe_psql('postgres',
+	"CREATE PUBLICATION tap_pub FOR TABLE conf_tab");
+
+# Insert row to be updated later
+$node_publisher->safe_psql('postgres',
+	"INSERT INTO conf_tab(a, data) VALUES (1,'frompub')");
+
+# Create the subscription
+my $appname = 'tap_sub';
+$node_subscriber->safe_psql(
+	'postgres',
+	"CREATE SUBSCRIPTION tap_sub
+	 CONNECTION '$publisher_connstr application_name=$appname'
+	 PUBLICATION tap_pub");
+
+# Wait for initial table sync to finish
+$node_subscriber->wait_for_subscription_sync($node_publisher, $appname);
+
+############################################
+# Race condition because of DirtySnapshot
+############################################
+
+my $psql_session_subscriber = $node_subscriber->background_psql('postgres');
+if ($simulate_race_condition)
+{
+	$node_subscriber->safe_psql('postgres', "SELECT injection_points_attach('index_getnext_slot_before_fetch_apply_dirty', 'wait')");
+}
+
+my $log_offset = -s $node_subscriber->logfile;
+
+# Update tuple on publisher
+$node_publisher->safe_psql('postgres',
+	"UPDATE conf_tab SET data = 'frompubnew' WHERE (a=1);");
+
+
+if ($simulate_race_condition)
+{
+	# Wait apply worker to start the search for the tuple using index
+	$node_subscriber->wait_for_event('logical replication apply worker', 'index_getnext_slot_before_fetch_apply_dirty');
+}
+
+# Update additional(!) column on the subscriber
+$psql_session_subscriber->query_until(
+	qr/start/, qq[
+	\\echo start
+	UPDATE conf_tab SET i = 1 WHERE (a=1);
+]);
+
+
+if ($simulate_race_condition)
+{
+	# Wake up apply worker
+	$node_subscriber->safe_psql('postgres',"
+		SELECT injection_points_detach('index_getnext_slot_before_fetch_apply_dirty');
+		SELECT injection_points_wakeup('index_getnext_slot_before_fetch_apply_dirty');
+		");
+}
+
+# Tuple was updated - so, we have conflict
+$node_subscriber->wait_for_log(
+	qr/conflict detected on relation \"public.conf_tab\"/,
+	$log_offset);
+
+$node_publisher->wait_for_catchup($appname);
+
+# We need new column value be synced with subscriber
+is($node_subscriber->safe_psql('postgres', 'SELECT data from conf_tab WHERE a = 1'), 'frompubnew', 'record updated on subscriber');
+# And additional column maintain updated value
+is($node_subscriber->safe_psql('postgres', 'SELECT i from conf_tab WHERE a = 1'), 1, 'column record updated on subscriber');
+
+ok(!$node_subscriber->log_contains(
+		qr/LOG:  conflict detected on relation \"public.conf_tab\": conflict=update_missing/,
+		$log_offset), 'invalid conflict detected');
+
+ok($node_subscriber->log_contains(
+		qr/LOG:  conflict detected on relation \"public.conf_tab\": conflict=update_origin_differs/,
+		$log_offset), 'correct conflict detected');
+
+done_testing();
diff --git a/src/test/subscription/t/038_update_missing_with_retain.pl b/src/test/subscription/t/038_update_missing_with_retain.pl
new file mode 100644
index 00000000000..7b225d45f7f
--- /dev/null
+++ b/src/test/subscription/t/038_update_missing_with_retain.pl
@@ -0,0 +1,143 @@
+# Copyright (c) 2025, PostgreSQL Global Development Group
+
+# Test the conflict detection and resolution in logical replication
+use strict;
+use warnings FATAL => 'all';
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+if ($ENV{enable_injection_points} ne 'yes')
+{
+	plan skip_all => 'Injection points not supported by this build';
+}
+
+############################## Set it to 0 to make set success; TODO: delete that for commit
+my $simulate_race_condition = 1;
+##############################
+
+###############################
+# Setup
+###############################
+
+# Initialize publisher node
+my $node_publisher = PostgreSQL::Test::Cluster->new('publisher');
+$node_publisher->init(allows_streaming => 'logical');
+$node_publisher->append_conf('postgresql.conf',
+	qq(track_commit_timestamp = on));
+$node_publisher->start;
+
+
+# Create subscriber node with track_commit_timestamp enabled
+my $node_subscriber = PostgreSQL::Test::Cluster->new('subscriber');
+$node_subscriber->init;
+$node_subscriber->append_conf('postgresql.conf',
+	qq(track_commit_timestamp = on));
+$node_subscriber->append_conf('postgresql.conf',
+	qq(wal_level = 'replica'));
+$node_subscriber->start;
+
+
+# Check if the extension injection_points is available, as it may be
+# possible that this script is run with installcheck, where the module
+# would not be installed by default.
+if (!$node_subscriber->check_extension('injection_points'))
+{
+	plan skip_all => 'Extension injection_points not installed';
+}
+
+# Create table on publisher
+$node_publisher->safe_psql(
+	'postgres',
+	"CREATE TABLE conf_tab(a int PRIMARY key, data text);");
+
+# Create similar table on subscriber with additional index to disable HOT updates and additional column
+$node_subscriber->safe_psql(
+	'postgres',
+	"CREATE TABLE conf_tab(a int PRIMARY key, data text, i int DEFAULT 0);
+	 CREATE INDEX i_index ON conf_tab(i);");
+
+# Set up extension to simulate race condition
+$node_subscriber->safe_psql('postgres', 'CREATE EXTENSION injection_points;');
+
+# Setup logical replication
+my $publisher_connstr = $node_publisher->connstr . ' dbname=postgres';
+$node_publisher->safe_psql('postgres',
+	"CREATE PUBLICATION tap_pub FOR TABLE conf_tab");
+
+# Insert row to be updated later
+$node_publisher->safe_psql('postgres',
+	"INSERT INTO conf_tab(a, data) VALUES (1,'frompub')");
+
+# Create the subscription
+my $appname = 'tap_sub';
+$node_subscriber->safe_psql(
+	'postgres',
+	"CREATE SUBSCRIPTION tap_sub
+	 CONNECTION '$publisher_connstr application_name=$appname'
+	 PUBLICATION tap_pub WITH (retain_dead_tuples = true)");
+
+# Wait for initial table sync to finish
+$node_subscriber->wait_for_subscription_sync($node_publisher, $appname);
+
+############################################
+# Race condition because of DirtySnapshot
+############################################
+
+my $psql_session_subscriber = $node_subscriber->background_psql('postgres');
+if ($simulate_race_condition)
+{
+	$node_subscriber->safe_psql('postgres', "SELECT injection_points_attach('index_getnext_slot_before_fetch_apply_dirty', 'wait')");
+}
+
+my $log_offset = -s $node_subscriber->logfile;
+
+# Update tuple on publisher
+$node_publisher->safe_psql('postgres',
+	"UPDATE conf_tab SET data = 'frompubnew' WHERE (a=1);");
+
+
+if ($simulate_race_condition)
+{
+	# Wait apply worker to start the search for the tuple using index
+	$node_subscriber->wait_for_event('logical replication apply worker', 'index_getnext_slot_before_fetch_apply_dirty');
+}
+
+# Update additional(!) column on the subscriber
+$psql_session_subscriber->query_until(
+	qr/start/, qq[
+	\\echo start
+	UPDATE conf_tab SET i = 1 WHERE (a=1);
+]);
+
+
+if ($simulate_race_condition)
+{
+	# Wake up apply worker
+	$node_subscriber->safe_psql('postgres',"
+		SELECT injection_points_detach('index_getnext_slot_before_fetch_apply_dirty');
+		SELECT injection_points_wakeup('index_getnext_slot_before_fetch_apply_dirty');
+		");
+}
+
+# Tuple was updated - so, we have conflict
+$node_subscriber->wait_for_log(
+	qr/conflict detected on relation \"public.conf_tab\"/,
+	$log_offset);
+
+$node_publisher->wait_for_catchup($appname);
+
+# We need new column value be synced with subscriber
+is($node_subscriber->safe_psql('postgres', 'SELECT data from conf_tab WHERE a = 1'), 'frompubnew', 'record updated on subscriber');
+# And additional column maintain updated value
+is($node_subscriber->safe_psql('postgres', 'SELECT i from conf_tab WHERE a = 1'), 1, 'column record updated on subscriber');
+
+ok(!$node_subscriber->log_contains(
+		qr/LOG:  conflict detected on relation \"public.conf_tab\": conflict=update_deleted/,
+		$log_offset), 'invalid conflict detected');
+
+ok($node_subscriber->log_contains(
+		qr/LOG:  conflict detected on relation \"public.conf_tab\": conflict=update_origin_differs/,
+		$log_offset), 'correct conflict detected');
+
+done_testing();
diff --git a/src/test/subscription/t/039_update_missing_simulation.pl b/src/test/subscription/t/039_update_missing_simulation.pl
new file mode 100644
index 00000000000..21fcd1ceb53
--- /dev/null
+++ b/src/test/subscription/t/039_update_missing_simulation.pl
@@ -0,0 +1,125 @@
+# Copyright (c) 2025, PostgreSQL Global Development Group
+
+# Test the conflict detection and resolution in logical replication
+# Not intended to be committed because quite heavy
+# Here to demonstrate reproducibility with pgbench
+use strict;
+use warnings FATAL => 'all';
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use IPC::Run qw(start finish);
+use Test::More;
+
+if ($ENV{enable_injection_points} ne 'yes')
+{
+	plan skip_all => 'Injection points not supported by this build';
+}
+
+###############################
+# Setup
+###############################
+
+# Initialize publisher node
+my $node_publisher = PostgreSQL::Test::Cluster->new('publisher');
+$node_publisher->init(allows_streaming => 'logical');
+$node_publisher->append_conf('postgresql.conf',
+	qq(track_commit_timestamp = on));
+$node_publisher->start;
+
+# Create subscriber node with track_commit_timestamp enabled
+my $node_subscriber = PostgreSQL::Test::Cluster->new('subscriber');
+$node_subscriber->init;
+$node_subscriber->append_conf('postgresql.conf',
+	qq(track_commit_timestamp = on));
+$node_subscriber->start;
+
+# Check if the extension injection_points is available, as it may be
+# possible that this script is run with installcheck, where the module
+# would not be installed by default.
+if (!$node_subscriber->check_extension('injection_points'))
+{
+	plan skip_all => 'Extension injection_points not installed';
+}
+
+# Create table on publisher
+$node_publisher->safe_psql(
+	'postgres',
+	"CREATE TABLE tbl(a int PRIMARY key, data_pub int);");
+
+# Create similar table on subscriber with additional index to disable HOT updates
+$node_subscriber->safe_psql(
+	'postgres',
+	"CREATE TABLE tbl(a int PRIMARY key, data_pub int, data_sub int default 0);
+	 CREATE INDEX data_index ON tbl(data_pub);");
+
+# Setup logical replication
+my $publisher_connstr = $node_publisher->connstr . ' dbname=postgres';
+$node_publisher->safe_psql('postgres',
+	"CREATE PUBLICATION tap_pub FOR TABLE tbl");
+
+# Create the subscription
+my $appname = 'tap_sub';
+$node_subscriber->safe_psql(
+	'postgres',
+	"CREATE SUBSCRIPTION tap_sub
+	 CONNECTION '$publisher_connstr application_name=$appname'
+	 PUBLICATION tap_pub");
+
+my $num_rows = 10;
+my $num_updates = 10000;
+my $num_clients = 10;
+$node_publisher->safe_psql('postgres', "INSERT INTO tbl SELECT i, i * i FROM generate_series(1,$num_rows) i");
+
+# Wait for initial table sync to finish
+$node_subscriber->wait_for_subscription_sync($node_publisher, $appname);
+
+# Prepare small pgbench scripts as files
+my $sub_sql = $node_subscriber->basedir . '/sub_update.sql';
+my $pub_sql = $node_publisher->basedir . '/pub_delete.sql';
+
+open my $fh1, '>', $sub_sql or die $!;
+print $fh1 "\\set num random(1,$num_rows)\nUPDATE tbl SET data_sub = data_sub + 1 WHERE a = :num;\n";
+close $fh1;
+
+open my $fh2, '>', $pub_sql or die $!;
+print $fh2 "\\set num random(1,$num_rows)\nUPDATE tbl SET data_pub = data_pub + 1 WHERE a = :num;\n";
+close $fh2;
+
+my @sub_cmd = (
+	'pgbench',
+	'--no-vacuum', "--client=$num_clients", '--jobs=4', '--exit-on-abort', "--transactions=$num_updates",
+	'-p', $node_subscriber->port, '-h', $node_subscriber->host, '-f', $sub_sql, 'postgres'
+);
+
+my @pub_cmd = (
+	'pgbench',
+	'--no-vacuum', "--client=$num_clients", '--jobs=4', '--exit-on-abort', "--transactions=$num_updates",
+	'-p', $node_publisher->port, '-h', $node_publisher->host, '-f', $pub_sql, 'postgres'
+);
+
+$node_subscriber->safe_psql('postgres', 'CREATE EXTENSION injection_points;');
+# This should never happen
+$node_subscriber->safe_psql('postgres',
+		"SELECT injection_points_attach('apply_handle_update_internal_update_missing', 'error')");
+my $log_offset = -s $node_subscriber->logfile;
+
+# Start both concurrently
+my ($sub_out, $sub_err, $pub_out, $pub_err) = ('', '', '', '');
+my $sub_h = start \@sub_cmd, '>', \$sub_out, '2>', \$sub_err;
+my $pub_h = start \@pub_cmd, '>', \$pub_out, '2>', \$pub_err;
+
+# Wait for completion
+finish $sub_h;
+finish $pub_h;
+
+like($sub_out, qr/actually processed/, 'subscriber pgbench completed');
+like($pub_out, qr/actually processed/, 'publisher pgbench completed');
+
+# Let subscription catch up, then check expectations
+$node_subscriber->wait_for_subscription_sync($node_publisher, 'tap_sub');
+
+ok(!$node_subscriber->log_contains(
+		qr/ERROR:  error triggered for injection point apply_handle_update_internal_update_missing/,
+		$log_offset), 'invalid conflict detected');
+
+done_testing();
-- 
2.48.1

---

From: Mihail Nikalayeu <[email-masked]>
Date: Tue, 28 Oct 2025 20:44:00 +0100

And rebased :)
From 6a4e379df251d27e42b8191bd153691f4d2d9886 Mon Sep 17 00:00:00 2001
From: nkey <[email-masked]>
Date: Sat, 23 Nov 2024 13:25:11 +0100
Subject: [PATCH v16 1/2] This patch introduces new injection points and TAP
 tests to reproduce and verify conflict detection issues that arise during
 SNAPSHOT_DIRTY index scans in logical replication.

---
 src/backend/access/index/indexam.c            |   9 ++
 src/backend/access/nbtree/README              |   9 ++
 src/backend/executor/execIndexing.c           |   7 +-
 src/backend/replication/logical/worker.c      |   4 +
 src/include/utils/snapshot.h                  |  14 ++
 src/test/subscription/meson.build             |   4 +
 .../subscription/t/037_delete_missing_race.pl | 139 +++++++++++++++++
 .../subscription/t/038_update_missing_race.pl | 141 +++++++++++++++++
 .../t/039_update_missing_with_retain.pl       | 143 ++++++++++++++++++
 .../t/040_update_missing_simulation.pl        | 125 +++++++++++++++
 10 files changed, 594 insertions(+), 1 deletion(-)
 create mode 100644 src/test/subscription/t/037_delete_missing_race.pl
 create mode 100644 src/test/subscription/t/038_update_missing_race.pl
 create mode 100644 src/test/subscription/t/039_update_missing_with_retain.pl
 create mode 100644 src/test/subscription/t/040_update_missing_simulation.pl

index 0492d92d23b..5987d90ee08 100644

 

 
+	if (!conflict)
 
index 7edd1c9cf06..0f2ffc754c9 100644
@@ -286,6 +286,7 @@
@@ -2962,7 +2963,10 @@ apply_handle_update_internal(ApplyExecutionData *edata,
 
index 85d10a89994..b552ae60a88 100644
@@ -46,6 +46,10 @@ tests += {
       't/036_sequences.pl',
+	  't/037_delete_missing_race.pl',
+	  't/038_update_missing_race.pl',
+	  't/039_update_missing_with_retain.pl',
+	  't/040_update_missing_simulation.pl',
diff --git a/src/test/subscription/t/037_delete_missing_race.pl b/src/test/subscription/t/037_delete_missing_race.pl
+++ b/src/test/subscription/t/037_delete_missing_race.pl
diff --git a/src/test/subscription/t/038_update_missing_race.pl b/src/test/subscription/t/038_update_missing_race.pl
+++ b/src/test/subscription/t/038_update_missing_race.pl
diff --git a/src/test/subscription/t/039_update_missing_with_retain.pl b/src/test/subscription/t/039_update_missing_with_retain.pl
+++ b/src/test/subscription/t/039_update_missing_with_retain.pl
diff --git a/src/test/subscription/t/040_update_missing_simulation.pl b/src/test/subscription/t/040_update_missing_simulation.pl
+++ b/src/test/subscription/t/040_update_missing_simulation.pl

From fe065bae15156cce590271b2977f516eb2ae7568 Mon Sep 17 00:00:00 2001
From: nkey <[email-masked]>
Date: Wed, 3 Sep 2025 19:08:55 +0200
Subject: [PATCH v16 2/2] Fix logical replication conflict detection during
 tuple lookup

index def32774c90..1e434ab697a 100644

 
 
 retry:
 	found = false;
-

 

-		PopActiveSnapshot();
-
 
 	index_endscan(scan);
+	PopActiveSnapshot();

 

 retry:
 	found = false;

 

-		PopActiveSnapshot();

 	table_endscan(scan);
+	PopActiveSnapshot();
 
 	return found;
-- 
2.48.1

---

From: Mihail Nikalayeu <[email-masked]>
Date: Thu, 13 Nov 2025 13:22:38 +0100

Hello, Amit!

> replication document for now unless we have a consensus on a way to

I have added a draft of the documentation patch in related thread [0].

[0]: https://www.postgresql.org/message-id/flat/CADzfLwU_o%2BeL2z9ifkpW2%2BkSJEXEbXkTmomvoEw-UY9CzgOn1A%40mail.gmail.com#2919d50a20b530092ec7e6ec71709697

Mikhail.

---

From: Mihail Nikalayeu <[email-masked]>
Date: Mon, 9 Mar 2026 01:08:00 +0100

Hello!

Rebased.

---

From: Mihail Nikalayeu <[email-masked]>
Date: Mon, 6 Apr 2026 20:33:00 +0200

Rebased.