0001-Fix-missing-EvalPlanQual-recheck-for-TID-scans.v2.patch

From aefec8d280fe9960806a4d40de95135b7d033b9c Mon Sep 17 00:00:00 2001
From: Sophie Alpert <git@sophiebits.com>
Date: Sat, 6 Sep 2025 00:30:34 -0700
Subject: [PATCH] Fix missing EvalPlanQual recheck for TID scans

---
 src/backend/executor/nodeTidrangescan.c       | 33 ++++++++-
 src/backend/executor/nodeTidscan.c            | 19 +++--
 src/include/nodes/execnodes.h                 |  2 +
 .../isolation/expected/eval-plan-qual.out     | 70 +++++++++++++++++++
 src/test/isolation/specs/eval-plan-qual.spec  | 13 ++++
 5 files changed, 130 insertions(+), 7 deletions(-)

diff --git a/src/backend/executor/nodeTidrangescan.c b/src/backend/executor/nodeTidrangescan.c
index 26f7420b64b..d98e8809b71 100644
--- a/src/backend/executor/nodeTidrangescan.c
+++ b/src/backend/executor/nodeTidrangescan.c
@@ -128,7 +128,8 @@ TidExprListCreate(TidRangeScanState *tidrangestate)
  *		TidRangeEval
  *
  *		Compute and set node's block and offset range to scan by evaluating
- *		node->trss_tidexprs.  Returns false if we detect the range cannot
+ *		node->trss_tidexprs.  Returns false and sets trss_mintid/trss_maxtid
+ *		to be invalid ItemPointers if we detect that the range cannot
  *		contain any tuples.  Returns true if it's possible for the range to
  *		contain tuples.  We don't bother validating that trss_mintid is less
  *		than or equal to trss_maxtid, as the scan_set_tidrange() table AM
@@ -165,7 +166,16 @@ TidRangeEval(TidRangeScanState *node)
 
 		/* If the bound is NULL, *nothing* matches the qual. */
 		if (isNull)
+		{
+			ItemPointerSet(&node->trss_mintid, InvalidBlockNumber,
+						   InvalidOffsetNumber);
+			ItemPointerSet(&node->trss_maxtid, InvalidBlockNumber,
+						   InvalidOffsetNumber);
+
+			node->trss_boundsInitialized = true;
+
 			return false;
+		}
 
 		if (tidopexpr->exprtype == TIDEXPR_LOWER_BOUND)
 		{
@@ -207,6 +217,8 @@ TidRangeEval(TidRangeScanState *node)
 	ItemPointerCopy(&lowerBound, &node->trss_mintid);
 	ItemPointerCopy(&upperBound, &node->trss_maxtid);
 
+	node->trss_boundsInitialized = true;
+
 	return true;
 }
 
@@ -274,6 +286,23 @@ TidRangeNext(TidRangeScanState *node)
 static bool
 TidRangeRecheck(TidRangeScanState *node, TupleTableSlot *slot)
 {
+	if (!node->trss_boundsInitialized) {
+		if (!TidRangeEval(node))
+			return false;
+
+		/*
+		 * If TidRangeEval returned false, then subsequent calls of Recheck will
+		 * return false due to the bounds having InvalidOffsetNumber.
+		 */
+	}
+
+	Assert(ItemPointerIsValid(&slot->tts_tid));
+
+	/* Recheck the ctid is still within range */
+	if (ItemPointerCompare(&slot->tts_tid, &node->trss_mintid) < 0 ||
+		ItemPointerCompare(&slot->tts_tid, &node->trss_maxtid) > 0)
+		return false;
+
 	return true;
 }
 
@@ -311,6 +340,7 @@ ExecReScanTidRangeScan(TidRangeScanState *node)
 {
 	/* mark scan as not in progress, and tid range list as not computed yet */
 	node->trss_inScan = false;
+	node->trss_boundsInitialized = false;
 
 	/*
 	 * We must wait until TidRangeNext before calling table_rescan_tidrange.
@@ -370,6 +400,7 @@ ExecInitTidRangeScan(TidRangeScan *node, EState *estate, int eflags)
 	 * mark scan as not in progress, and TID range as not computed yet
 	 */
 	tidrangestate->trss_inScan = false;
+	tidrangestate->trss_boundsInitialized = false;
 
 	/*
 	 * open the scan relation
diff --git a/src/backend/executor/nodeTidscan.c b/src/backend/executor/nodeTidscan.c
index 5e56e29a15f..1bee0ef55e0 100644
--- a/src/backend/executor/nodeTidscan.c
+++ b/src/backend/executor/nodeTidscan.c
@@ -402,12 +402,19 @@ TidNext(TidScanState *node)
 static bool
 TidRecheck(TidScanState *node, TupleTableSlot *slot)
 {
-	/*
-	 * XXX shouldn't we check here to make sure tuple matches TID list? In
-	 * runtime-key case this is not certain, is it?  However, in the WHERE
-	 * CURRENT OF case it might not match anyway ...
-	 */
-	return true;
+	ItemPointer match;
+
+	/* WHERE CURRENT OF always intends to resolve to the latest tuple */
+	if (node->tss_isCurrentOf)
+		return true;
+
+	if (node->tss_TidList == NULL)
+		TidListEval(node);
+
+	match = (ItemPointer) bsearch(&slot->tts_tid, node->tss_TidList,
+								  node->tss_NumTids, sizeof(ItemPointerData),
+								  itemptr_comparator);
+	return match != NULL;
 }
 
 
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index de782014b2d..ec7020512d2 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -1927,6 +1927,7 @@ typedef struct TidScanState
  *		trss_mintid			the lowest TID in the scan range
  *		trss_maxtid			the highest TID in the scan range
  *		trss_inScan			is a scan currently in progress?
+ *		trss_boundsInitialized	has the scan range been computed?
  * ----------------
  */
 typedef struct TidRangeScanState
@@ -1936,6 +1937,7 @@ typedef struct TidRangeScanState
 	ItemPointerData trss_mintid;
 	ItemPointerData trss_maxtid;
 	bool		trss_inScan;
+	bool    trss_boundsInitialized;
 } TidRangeScanState;
 
 /* ----------------
diff --git a/src/test/isolation/expected/eval-plan-qual.out b/src/test/isolation/expected/eval-plan-qual.out
index 032d4208d51..f279a680d5b 100644
--- a/src/test/isolation/expected/eval-plan-qual.out
+++ b/src/test/isolation/expected/eval-plan-qual.out
@@ -1218,6 +1218,76 @@ subid|id
 (1 row)
 
 
+starting permutation: tid1 tid2 c1 c2 read
+step tid1: UPDATE accounts SET balance = balance + 100 WHERE ctid = '(0,1)' RETURNING accountid, balance;
+accountid|balance
+---------+-------
+checking |    700
+(1 row)
+
+step tid2: UPDATE accounts SET balance = balance + 200 WHERE ctid = '(0,1)' RETURNING accountid, balance; <waiting ...>
+step c1: COMMIT;
+step tid2: <... completed>
+accountid|balance
+---------+-------
+(0 rows)
+
+step c2: COMMIT;
+step read: SELECT * FROM accounts ORDER BY accountid;
+accountid|balance|balance2
+---------+-------+--------
+checking |    700|    1400
+savings  |    600|    1200
+(2 rows)
+
+
+starting permutation: tid1 tidsucceed2 c1 c2 read
+step tid1: UPDATE accounts SET balance = balance + 100 WHERE ctid = '(0,1)' RETURNING accountid, balance;
+accountid|balance
+---------+-------
+checking |    700
+(1 row)
+
+step tidsucceed2: UPDATE accounts SET balance = balance + 200 WHERE ctid = '(0,1)' OR ctid = '(0,3)' RETURNING accountid, balance; <waiting ...>
+step c1: COMMIT;
+step tidsucceed2: <... completed>
+accountid|balance
+---------+-------
+checking |    900
+(1 row)
+
+step c2: COMMIT;
+step read: SELECT * FROM accounts ORDER BY accountid;
+accountid|balance|balance2
+---------+-------+--------
+checking |    900|    1800
+savings  |    600|    1200
+(2 rows)
+
+
+starting permutation: tidrange1 tidrange2 c1 c2 read
+step tidrange1: UPDATE accounts SET balance = balance + 100 WHERE ctid BETWEEN '(0,1)' AND '(0,1)' RETURNING accountid, balance;
+accountid|balance
+---------+-------
+checking |    700
+(1 row)
+
+step tidrange2: UPDATE accounts SET balance = balance + 200 WHERE ctid BETWEEN '(0,1)' AND '(0,1)' RETURNING accountid, balance; <waiting ...>
+step c1: COMMIT;
+step tidrange2: <... completed>
+accountid|balance
+---------+-------
+(0 rows)
+
+step c2: COMMIT;
+step read: SELECT * FROM accounts ORDER BY accountid;
+accountid|balance|balance2
+---------+-------+--------
+checking |    700|    1400
+savings  |    600|    1200
+(2 rows)
+
+
 starting permutation: simplepartupdate conditionalpartupdate c1 c2 read_part
 step simplepartupdate: 
 	update parttbl set b = b + 10;
diff --git a/src/test/isolation/specs/eval-plan-qual.spec b/src/test/isolation/specs/eval-plan-qual.spec
index 07307e623e4..1498e775c0f 100644
--- a/src/test/isolation/specs/eval-plan-qual.spec
+++ b/src/test/isolation/specs/eval-plan-qual.spec
@@ -99,6 +99,11 @@ step upsert1	{
 	  WHERE NOT EXISTS (SELECT 1 FROM upsert);
 }
 
+# ctid predicate
+# same filter in both sessions; only one should succeed
+step tid1 { UPDATE accounts SET balance = balance + 100 WHERE ctid = '(0,1)' RETURNING accountid, balance; }
+step tidrange1 { UPDATE accounts SET balance = balance + 100 WHERE ctid BETWEEN '(0,1)' AND '(0,1)' RETURNING accountid, balance; }
+
 # tests with table p check inheritance cases:
 # readp1/writep1/readp2 tests a bug where nodeLockRows did the wrong thing
 # when the first updated tuple was in a non-first child table.
@@ -241,6 +246,11 @@ step updateforcip3	{
 step wrtwcte	{ UPDATE table_a SET value = 'tableAValue2' WHERE id = 1; }
 step wrjt	{ UPDATE jointest SET data = 42 WHERE id = 7; }
 
+step tid2 { UPDATE accounts SET balance = balance + 200 WHERE ctid = '(0,1)' RETURNING accountid, balance; }
+step tidrange2 { UPDATE accounts SET balance = balance + 200 WHERE ctid BETWEEN '(0,1)' AND '(0,1)' RETURNING accountid, balance; }
+# here, recheck succeeds; (0,3) is the id that step tid1 will assign
+step tidsucceed2 { UPDATE accounts SET balance = balance + 200 WHERE ctid = '(0,1)' OR ctid = '(0,3)' RETURNING accountid, balance; }
+
 step conditionalpartupdate	{
 	update parttbl set c = -c where b < 10;
 }
@@ -392,6 +402,9 @@ permutation wrtwcte readwcte c1 c2
 permutation wrjt selectjoinforupdate c2 c1
 permutation wrjt selectresultforupdate c2 c1
 permutation wrtwcte multireadwcte c1 c2
+permutation tid1 tid2 c1 c2 read
+permutation tid1 tidsucceed2 c1 c2 read
+permutation tidrange1 tidrange2 c1 c2 read
 
 permutation simplepartupdate conditionalpartupdate c1 c2 read_part
 permutation simplepartupdate complexpartupdate c1 c2 read_part
-- 
2.39.5 (Apple Git-154)