From aefec8d280fe9960806a4d40de95135b7d033b9c Mon Sep 17 00:00:00 2001 From: Sophie Alpert 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; +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; +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; +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)