Add missing EPQ recheck for TID Range Scan

The EvalPlanQual recheck for TID Range Scan wasn't rechecking the TID qual
still passed after following update chains.  This could result in tuples
being updated or deleted by plans using TID Range Scans where the ctid of
the new (updated) tuple no longer matches the clause of the scan.  This
isn't desired behavior, and isn't consistent with what would happen if the
chosen plan had used an Index or Seq Scan, and that could lead to hard to
predict behavior for scans that contain TID quals and other quals as the
planner has freedom to choose TID Range or some other non-TID scan method
for such queries, and the chosen plan could change at any moment.

Here we fix this by properly implementing the recheck function for TID
Range Scans.

Backpatch to 14, where TID Range Scans were added

Reported-by: Sophie Alpert <pg@sophiebits.com>
Author: Sophie Alpert <pg@sophiebits.com>
Author: David Rowley <dgrowleyml@gmail.com>
Reviewed-by: David Rowley <dgrowleyml@gmail.com>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Discussion: https://postgr.es/m/4a6268ff-3340-453a-9bf5-c98d51a6f729@app.fastmail.com
Backpatch-through: 14

diff --git a/src/backend/executor/nodeTidrangescan.c b/src/backend/executor/nodeTidrangescan.c
index 2b0d205d7dda28f8d0b19ef6a4f322ec2afcbafb..f75c33a099acb226d58c8cf8698550c54266ac35 100644 (file)
--- a/src/backend/executor/nodeTidrangescan.c
+++ b/src/backend/executor/nodeTidrangescan.c
@@ -268,6 +268,16 @@ TidRangeNext(TidRangeScanState *node)
 static bool
 TidRangeRecheck(TidRangeScanState *node, TupleTableSlot *slot)
 {
+   if (!TidRangeEval(node))
+       return false;
+
+   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;
 }
 

diff --git a/src/test/isolation/expected/eval-plan-qual.out b/src/test/isolation/expected/eval-plan-qual.out
index f4c2594345d8c1bb938811b8ccc9cdfae06efef7..00c1604e90643939ae17f2c09f0704d1c0dbc562 100644 (file)
--- a/src/test/isolation/expected/eval-plan-qual.out
+++ b/src/test/isolation/expected/eval-plan-qual.out
@@ -1235,6 +1235,29 @@ 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: tid1 tid2 r1 c2 read
 step tid1: UPDATE accounts SET balance = balance + 100 WHERE ctid = '(0,1)' RETURNING accountid, balance;
 accountid|balance

diff --git a/src/test/isolation/specs/eval-plan-qual.spec b/src/test/isolation/specs/eval-plan-qual.spec
index 474985f7e29c1898177b63223815b57e2359ca97..f685392956ed9065e31e2acfc15994028bfcad61 100644 (file)
--- a/src/test/isolation/specs/eval-plan-qual.spec
+++ b/src/test/isolation/specs/eval-plan-qual.spec
@@ -95,8 +95,9 @@ step upsert1  {
      WHERE NOT EXISTS (SELECT 1 FROM upsert);
 }
 
-# Tests for Tid Scan
+# Tests for Tid / Tid Range Scan
 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
@@ -241,6 +242,7 @@ 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; }
 
@@ -388,6 +390,7 @@ 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
 # test that a rollback on s1 has s2 perform the update on the original row
 permutation tid1 tid2 r1 c2 read