Fix misbehavior of EvalPlanQual checks with multiple result relations.

tglsfdc · tglsfdc · commit ff77d8687fa3 · 2023-05-19T14:26:34.000-04:00
The idea of EvalPlanQual is that we replace the query's scan of the result relation with a single injected tuple, and see if we get a tuple out, thereby implying that the injected tuple still passes the query quals. (In join cases, other relations in the query are still scanned normally.) This logic was not updated when commit 86dc900 made it possible for a single DML query plan to have multiple result relations, when the query target relation has inheritance or partition children. We replaced the output for the current result relation successfully, but other result relations were still scanned normally; thus, if any other result relation contained a tuple satisfying the quals, we'd think the EPQ check passed, even if it did not pass for the injected tuple itself. This would lead to update or delete actions getting performed when they should have been skipped due to a conflicting concurrent update in READ COMMITTED isolation mode. Fix by blocking all sibling result relations from emitting tuples during an EvalPlanQual recheck. In the back branches, the fix is complicated a bit by the need to not change the size of struct EPQState (else we'd have ABI-breaking changes in offsets in struct ModifyTableState). Like the back-patches of 3f7836f and 4b3e379, add a separately palloc'd struct to avoid that. The logic is the same as in HEAD otherwise. This is only a live bug back to v14 where 86dc900 came in. However, I chose to back-patch the test cases further, on the grounds that this whole area is none too well tested. I skipped doing so in v11 though because none of the test applied cleanly, and it didn't quite seem worth extra work for a branch with only six months to live. Per report from Ante Krešić (via Aleksander Alekseev) Discussion: https://postgr.es/m/CAJ7c6TMBTN3rcz4=AjYhLPD_w3FFT0Wq_C15jxCDn8U4tZnH1g@mail.gmail.com
diff --git a/src/test/isolation/expected/eval-plan-qual.out b/src/test/isolation/expected/eval-plan-qual.out
@@ -842,6 +842,90 @@ step c1: COMMIT;
 step writep3b: <... completed>
 step c2: COMMIT;
 
+starting permutation: writep4a writep4b c1 c2 readp
+step writep4a: UPDATE p SET c = 4 WHERE c = 0;
+step writep4b: UPDATE p SET b = -4 WHERE c = 0; <waiting ...>
+step c1: COMMIT;
+step writep4b: <... completed>
+step c2: COMMIT;
+step readp: SELECT tableoid::regclass, ctid, * FROM p;
+tableoid|ctid  |a|b|c
+--------+------+-+-+-
+c1      |(0,2) |0|0|1
+c1      |(0,3) |0|0|2
+c1      |(0,5) |0|1|1
+c1      |(0,6) |0|1|2
+c1      |(0,8) |0|2|1
+c1      |(0,9) |0|2|2
+c1      |(0,11)|0|0|4
+c1      |(0,12)|0|1|4
+c1      |(0,13)|0|2|4
+c1      |(0,14)|0|3|4
+c2      |(0,2) |1|0|1
+c2      |(0,3) |1|0|2
+c2      |(0,5) |1|1|1
+c2      |(0,6) |1|1|2
+c2      |(0,8) |1|2|1
+c2      |(0,9) |1|2|2
+c2      |(0,11)|1|0|4
+c2      |(0,12)|1|1|4
+c2      |(0,13)|1|2|4
+c2      |(0,14)|1|3|4
+c3      |(0,2) |2|0|1
+c3      |(0,3) |2|0|2
+c3      |(0,5) |2|1|1
+c3      |(0,6) |2|1|2
+c3      |(0,8) |2|2|1
+c3      |(0,9) |2|2|2
+c3      |(0,11)|2|0|4
+c3      |(0,12)|2|1|4
+c3      |(0,13)|2|2|4
+c3      |(0,14)|2|3|4
+(30 rows)
+
+
+starting permutation: writep4a deletep4 c1 c2 readp
+step writep4a: UPDATE p SET c = 4 WHERE c = 0;
+step deletep4: DELETE FROM p WHERE c = 0; <waiting ...>
+step c1: COMMIT;
+step deletep4: <... completed>
+step c2: COMMIT;
+step readp: SELECT tableoid::regclass, ctid, * FROM p;
+tableoid|ctid  |a|b|c
+--------+------+-+-+-
+c1      |(0,2) |0|0|1
+c1      |(0,3) |0|0|2
+c1      |(0,5) |0|1|1
+c1      |(0,6) |0|1|2
+c1      |(0,8) |0|2|1
+c1      |(0,9) |0|2|2
+c1      |(0,11)|0|0|4
+c1      |(0,12)|0|1|4
+c1      |(0,13)|0|2|4
+c1      |(0,14)|0|3|4
+c2      |(0,2) |1|0|1
+c2      |(0,3) |1|0|2
+c2      |(0,5) |1|1|1
+c2      |(0,6) |1|1|2
+c2      |(0,8) |1|2|1
+c2      |(0,9) |1|2|2
+c2      |(0,11)|1|0|4
+c2      |(0,12)|1|1|4
+c2      |(0,13)|1|2|4
+c2      |(0,14)|1|3|4
+c3      |(0,2) |2|0|1
+c3      |(0,3) |2|0|2
+c3      |(0,5) |2|1|1
+c3      |(0,6) |2|1|2
+c3      |(0,8) |2|2|1
+c3      |(0,9) |2|2|2
+c3      |(0,11)|2|0|4
+c3      |(0,12)|2|1|4
+c3      |(0,13)|2|2|4
+c3      |(0,14)|2|3|4
+(30 rows)
+
+
 starting permutation: wx2 partiallock c2 c1 read
 step wx2: UPDATE accounts SET balance = balance + 450 WHERE accountid = 'checking' RETURNING balance;
 balance
@@ -1104,22 +1188,41 @@ subid|id
 (1 row)
 
 
+starting permutation: simplepartupdate conditionalpartupdate c1 c2 read_part
+step simplepartupdate: 
+	update parttbl set b = b + 10;
+
+step conditionalpartupdate: 
+	update parttbl set c = -c where b < 10;
+ <waiting ...>
+step c1: COMMIT;
+step conditionalpartupdate: <... completed>
+step c2: COMMIT;
+step read_part: SELECT * FROM parttbl ORDER BY a, c;
+a| b|c| d
+-+--+-+--
+1|11|1|12
+2|12|2|14
+(2 rows)
+
+
 starting permutation: simplepartupdate complexpartupdate c1 c2 read_part
 step simplepartupdate: 
 	update parttbl set b = b + 10;
 
 step complexpartupdate: 
 	with u as (update parttbl set b = b + 1 returning parttbl.*)
-	update parttbl set b = u.b + 100 from u;
+	update parttbl p set b = u.b + 100 from u where p.a = u.a;
  <waiting ...>
 step c1: COMMIT;
 step complexpartupdate: <... completed>
 step c2: COMMIT;
-step read_part: SELECT * FROM parttbl ORDER BY a;
+step read_part: SELECT * FROM parttbl ORDER BY a, c;
 a| b|c| d
 -+--+-+--
 1|12|1|13
-(1 row)
+2|13|2|15
+(2 rows)
 
 
 starting permutation: simplepartupdate_route1to2 complexpartupdate_route_err1 c1 c2 read_part
@@ -1139,11 +1242,12 @@ step c1: COMMIT;
 step complexpartupdate_route_err1: <... completed>
 ERROR:  tuple to be locked was already moved to another partition due to concurrent update
 step c2: COMMIT;
-step read_part: SELECT * FROM parttbl ORDER BY a;
+step read_part: SELECT * FROM parttbl ORDER BY a, c;
 a|b|c|d
 -+-+-+-
 2|1|1|3
-(1 row)
+2|2|2|4
+(2 rows)
 
 
 starting permutation: simplepartupdate_noroute complexpartupdate_route c1 c2 read_part
@@ -1167,11 +1271,12 @@ a|b|c|d
 (1 row)
 
 step c2: COMMIT;
-step read_part: SELECT * FROM parttbl ORDER BY a;
+step read_part: SELECT * FROM parttbl ORDER BY a, c;
 a|b|c|d
 -+-+-+-
 2|2|1|4
-(1 row)
+2|2|2|4
+(2 rows)
 
 
 starting permutation: simplepartupdate_noroute complexpartupdate_doesnt_route c1 c2 read_part
@@ -1195,9 +1300,10 @@ a|b|c|d
 (1 row)
 
 step c2: COMMIT;
-step read_part: SELECT * FROM parttbl ORDER BY a;
+step read_part: SELECT * FROM parttbl ORDER BY a, c;
 a|b|c|d
 -+-+-+-
 1|2|1|3
-(1 row)
+2|2|2|4
+(2 rows)
 
diff --git a/src/test/isolation/specs/eval-plan-qual.spec b/src/test/isolation/specs/eval-plan-qual.spec
@@ -38,7 +38,7 @@ setup
    d int GENERATED ALWAYS AS (a + b) STORED) PARTITION BY LIST (a);
  CREATE TABLE parttbl1 PARTITION OF parttbl FOR VALUES IN (1);
  CREATE TABLE parttbl2 PARTITION OF parttbl FOR VALUES IN (2);
- INSERT INTO parttbl VALUES (1, 1, 1);
+ INSERT INTO parttbl VALUES (1, 1, 1), (2, 2, 2);
 
  CREATE TABLE another_parttbl (a int, b int, c int) PARTITION BY LIST (a);
  CREATE TABLE another_parttbl1 PARTITION OF another_parttbl FOR VALUES IN (1);
@@ -100,11 +100,15 @@ step upsert1	{
 # when the first updated tuple was in a non-first child table.
 # writep2/returningp1 tests a memory allocation issue
 # writep3a/writep3b tests updates touching more than one table
+# writep4a/writep4b tests a case where matches in another table confused EPQ
+# writep4a/deletep4 tests the same case in the DELETE path
 
+step readp		{ SELECT tableoid::regclass, ctid, * FROM p; }
 step readp1		{ SELECT tableoid::regclass, ctid, * FROM p WHERE b IN (0, 1) AND c = 0 FOR UPDATE; }
 step writep1	{ UPDATE p SET b = -1 WHERE a = 1 AND b = 1 AND c = 0; }
 step writep2	{ UPDATE p SET b = -b WHERE a = 1 AND c = 0; }
 step writep3a	{ UPDATE p SET b = -b WHERE c = 0; }
+step writep4a	{ UPDATE p SET c = 4 WHERE c = 0; }
 step c1		{ COMMIT; }
 step r1		{ ROLLBACK; }
 
@@ -208,6 +212,8 @@ step returningp1 {
 	  SELECT * FROM u;
 }
 step writep3b	{ UPDATE p SET b = -b WHERE c = 0; }
+step writep4b	{ UPDATE p SET b = -4 WHERE c = 0; }
+step deletep4	{ DELETE FROM p WHERE c = 0; }
 step readforss	{
 	SELECT ta.id AS ta_id, ta.value AS ta_value,
 		(SELECT ROW(tb.id, tb.value)
@@ -224,9 +230,14 @@ step updateforcip3	{
 }
 step wrtwcte	{ UPDATE table_a SET value = 'tableAValue2' WHERE id = 1; }
 step wrjt	{ UPDATE jointest SET data = 42 WHERE id = 7; }
+
+step conditionalpartupdate	{
+	update parttbl set c = -c where b < 10;
+}
+
 step complexpartupdate	{
 	with u as (update parttbl set b = b + 1 returning parttbl.*)
-	update parttbl set b = u.b + 100 from u;
+	update parttbl p set b = u.b + 100 from u where p.a = u.a;
 }
 
 step complexpartupdate_route_err1 {
@@ -275,7 +286,7 @@ setup		{ BEGIN ISOLATION LEVEL READ COMMITTED; SET client_min_messages = 'WARNIN
 step read	{ SELECT * FROM accounts ORDER BY accountid; }
 step read_ext	{ SELECT * FROM accounts_ext ORDER BY accountid; }
 step read_a	{ SELECT * FROM table_a ORDER BY id; }
-step read_part	{ SELECT * FROM parttbl ORDER BY a; }
+step read_part	{ SELECT * FROM parttbl ORDER BY a, c; }
 
 # this test exercises EvalPlanQual with a CTE, cf bug #14328
 step readwcte	{
@@ -345,6 +356,8 @@ permutation upsert1 upsert2 c1 c2 read
 permutation readp1 writep1 readp2 c1 c2
 permutation writep2 returningp1 c1 c2
 permutation writep3a writep3b c1 c2
+permutation writep4a writep4b c1 c2 readp
+permutation writep4a deletep4 c1 c2 readp
 permutation wx2 partiallock c2 c1 read
 permutation wx2 lockwithvalues c2 c1 read
 permutation wx2_ext partiallock_ext c2 c1 read_ext
@@ -356,6 +369,7 @@ permutation wrjt selectjoinforupdate c2 c1
 permutation wrjt selectresultforupdate c2 c1
 permutation wrtwcte multireadwcte c1 c2
 
+permutation simplepartupdate conditionalpartupdate c1 c2 read_part
 permutation simplepartupdate complexpartupdate c1 c2 read_part
 permutation simplepartupdate_route1to2 complexpartupdate_route_err1 c1 c2 read_part
 permutation simplepartupdate_noroute complexpartupdate_route c1 c2 read_part
