From 23b75dd03da1a4a6db080b27bb3c8efcf7dbc9ae Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Mon, 6 Mar 2023 18:31:16 -0500 Subject: [PATCH] Fix some more cases of missed GENERATED-column updates. If UPDATE is forced to retry after an EvalPlanQual check, it neglected to repeat GENERATED-column computations, even though those might well have changed since we're dealing with a different tuple than before. Fixing this is mostly a matter of looping back a bit further when we retry. In v15 and HEAD that's most easily done by altering the API of ExecUpdateAct so that it includes computing GENERATED expressions. Also, if an UPDATE in a partitioned table turns into a cross-partition INSERT operation, we failed to recompute GENERATED columns. That's a bug since 8bf6ec3ba allowed partitions to have different generation expressions; although it seems to have no ill effects before that. Fixing this is messier because we can now have situations where the same query needs both the UPDATE-aligned set of GENERATED columns and the INSERT-aligned set, and it's unclear which set will be generated first (else we could hack things by forcing the INSERT-aligned set to be generated, which is indeed how fe9e658f4 made it work for MERGE). The best fix seems to be to build and store separate sets of expressions for the INSERT and UPDATE cases. That would create ABI issues in the back branches, but so far it seems we can leave this alone in the back branches. Per bug #17823 from Hisahiro Kauchi. The first part of this affects all branches back to v12 where GENERATED columns were added. Discussion: https://postgr.es/m/17823-b64909cf7d63de84@postgresql.org --- src/backend/executor/nodeModifyTable.c | 21 +- .../isolation/expected/eval-plan-qual.out | 304 ++++++++++-------- src/test/isolation/specs/eval-plan-qual.spec | 21 +- src/test/regress/expected/generated.out | 14 + src/test/regress/sql/generated.sql | 4 + 5 files changed, 204 insertions(+), 160 deletions(-) diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c index 5d1c5d41ed..e64b355eec 100644 --- a/src/backend/executor/nodeModifyTable.c +++ b/src/backend/executor/nodeModifyTable.c @@ -1193,6 +1193,15 @@ ExecUpdate(ModifyTableState *mtstate, bool partition_constraint_failed; bool update_indexes; + /* + * If we generate a new candidate tuple after EvalPlanQual testing, we + * must loop back here to try again. (We don't need to redo triggers, + * however. If there are any BEFORE triggers then trigger.c will have + * done table_tuple_lock to lock the correct tuple, so there's no need + * to do them again.) + */ +lreplace: + /* * Constraints and GENERATED expressions might reference the tableoid * column, so (re-)initialize tts_tableOid before evaluating them. @@ -1206,17 +1215,6 @@ ExecUpdate(ModifyTableState *mtstate, resultRelationDesc->rd_att->constr->has_generated_stored) ExecComputeStoredGenerated(estate, slot); - /* - * Check any RLS UPDATE WITH CHECK policies - * - * If we generate a new candidate tuple after EvalPlanQual testing, we - * must loop back here and recheck any RLS policies and constraints. - * (We don't need to redo triggers, however. If there are any BEFORE - * triggers then trigger.c will have done table_tuple_lock to lock the - * correct tuple, so there's no need to do them again.) - */ -lreplace:; - /* ensure slot is independent, consider e.g. EPQ */ ExecMaterializeSlot(slot); @@ -1231,6 +1229,7 @@ lreplace:; resultRelInfo->ri_PartitionCheck && !ExecPartitionCheck(resultRelInfo, slot, estate, false); + /* Check any RLS UPDATE WITH CHECK policies */ if (!partition_constraint_failed && resultRelInfo->ri_WithCheckOptions != NIL) { diff --git a/src/test/isolation/expected/eval-plan-qual.out b/src/test/isolation/expected/eval-plan-qual.out index 4333f9541c..a2ffbd82d5 100644 --- a/src/test/isolation/expected/eval-plan-qual.out +++ b/src/test/isolation/expected/eval-plan-qual.out @@ -17,10 +17,10 @@ balance step c2: COMMIT; step read: SELECT * FROM accounts ORDER BY accountid; -accountid|balance ----------+------- -checking | 850 -savings | 600 +accountid|balance|balance2 +---------+-------+-------- +checking | 850| 1700 +savings | 600| 1200 (2 rows) @@ -40,10 +40,10 @@ balance step c2: COMMIT; step read: SELECT * FROM accounts ORDER BY accountid; -accountid|balance ----------+------- -checking | 1100 -savings | 600 +accountid|balance|balance2 +---------+-------+-------- +checking | 1100| 2200 +savings | 600| 1200 (2 rows) @@ -64,10 +64,10 @@ balance step c2: COMMIT; step read: SELECT * FROM accounts ORDER BY accountid; -accountid|balance ----------+------- -checking | 1050 -savings | 600 +accountid|balance|balance2 +---------+-------+-------- +checking | 1050| 2100 +savings | 600| 1200 (2 rows) @@ -88,10 +88,10 @@ balance step c2: COMMIT; step read: SELECT * FROM accounts ORDER BY accountid; -accountid|balance ----------+------- -checking | 1600 -savings | 600 +accountid|balance|balance2 +---------+-------+-------- +checking | 1600| 3200 +savings | 600| 1200 (2 rows) @@ -117,9 +117,9 @@ balance step c2: COMMIT; step read: SELECT * FROM accounts ORDER BY accountid; -accountid|balance ----------+------- -savings | 600 +accountid|balance|balance2 +---------+-------+-------- +savings | 600| 1200 (1 row) @@ -140,9 +140,9 @@ balance step c1: COMMIT; step read: SELECT * FROM accounts ORDER BY accountid; -accountid|balance ----------+------- -savings | 600 +accountid|balance|balance2 +---------+-------+-------- +savings | 600| 1200 (1 row) @@ -168,10 +168,10 @@ balance step c1: COMMIT; step read: SELECT * FROM accounts ORDER BY accountid; -accountid|balance ----------+------- -checking | 1500 -savings | 600 +accountid|balance|balance2 +---------+-------+-------- +checking | 1500| 3000 +savings | 600| 1200 (2 rows) @@ -192,9 +192,9 @@ balance step c1: COMMIT; step read: SELECT * FROM accounts ORDER BY accountid; -accountid|balance ----------+------- -savings | 600 +accountid|balance|balance2 +---------+-------+-------- +savings | 600| 1200 (1 row) @@ -221,10 +221,10 @@ balance step c2: COMMIT; step read: SELECT * FROM accounts ORDER BY accountid; -accountid|balance ----------+------- -checking | 1050 -savings | 600 +accountid|balance|balance2 +---------+-------+-------- +checking | 1050| 2100 +savings | 600| 1200 (2 rows) @@ -245,9 +245,9 @@ balance step c1: COMMIT; step read: SELECT * FROM accounts ORDER BY accountid; -accountid|balance ----------+------- -savings | 600 +accountid|balance|balance2 +---------+-------+-------- +savings | 600| 1200 (1 row) @@ -274,9 +274,9 @@ balance step c1: COMMIT; step read: SELECT * FROM accounts ORDER BY accountid; -accountid|balance ----------+------- -savings | 600 +accountid|balance|balance2 +---------+-------+-------- +savings | 600| 1200 (1 row) @@ -298,9 +298,9 @@ balance step c1: COMMIT; step read: SELECT * FROM accounts ORDER BY accountid; -accountid|balance ----------+------- -savings | 600 +accountid|balance|balance2 +---------+-------+-------- +savings | 600| 1200 (1 row) @@ -320,9 +320,9 @@ balance step c2: COMMIT; step read: SELECT * FROM accounts ORDER BY accountid; -accountid|balance ----------+------- -savings | 600 +accountid|balance|balance2 +---------+-------+-------- +savings | 600| 1200 (1 row) @@ -343,10 +343,10 @@ balance step c2: COMMIT; step read: SELECT * FROM accounts ORDER BY accountid; -accountid|balance ----------+------- -checking | 1050 -savings | 600 +accountid|balance|balance2 +---------+-------+-------- +checking | 1050| 2100 +savings | 600| 1200 (2 rows) @@ -371,10 +371,10 @@ step wnested2: step c1: COMMIT; step c2: COMMIT; step read: SELECT * FROM accounts ORDER BY accountid; -accountid|balance ----------+------- -checking | -600 -savings | 600 +accountid|balance|balance2 +---------+-------+-------- +checking | -600| -1200 +savings | 600| 1200 (2 rows) @@ -420,10 +420,10 @@ s2: WARNING: upid: text savings = text checking: f step wnested2: <... completed> step c2: COMMIT; step read: SELECT * FROM accounts ORDER BY accountid; -accountid|balance ----------+------- -checking | -800 -savings | 600 +accountid|balance|balance2 +---------+-------+-------- +checking | -800| -1600 +savings | 600| 1200 (2 rows) @@ -471,10 +471,10 @@ s2: WARNING: upid: text savings = text checking: f step wnested2: <... completed> step c2: COMMIT; step read: SELECT * FROM accounts ORDER BY accountid; -accountid|balance ----------+------- -checking | 200 -savings | 600 +accountid|balance|balance2 +---------+-------+-------- +checking | 200| 400 +savings | 600| 1200 (2 rows) @@ -527,10 +527,10 @@ s2: WARNING: upid: text savings = text checking: f step wnested2: <... completed> step c2: COMMIT; step read: SELECT * FROM accounts ORDER BY accountid; -accountid|balance ----------+------- -checking | 200 -savings | 600 +accountid|balance|balance2 +---------+-------+-------- +checking | 200| 400 +savings | 600| 1200 (2 rows) @@ -577,10 +577,10 @@ s2: WARNING: upid: text savings = text checking: f step wnested2: <... completed> step c2: COMMIT; step read: SELECT * FROM accounts ORDER BY accountid; -accountid|balance ----------+------- -checking | 400 -savings | 600 +accountid|balance|balance2 +---------+-------+-------- +checking | 400| 800 +savings | 600| 1200 (2 rows) @@ -614,10 +614,10 @@ s2: WARNING: upid: text savings = text checking: f step wnested2: <... completed> step c2: COMMIT; step read: SELECT * FROM accounts ORDER BY accountid; -accountid|balance ----------+------- -cds | 400 -savings | 600 +accountid|balance|balance2 +---------+-------+-------- +cds | 400| 800 +savings | 600| 1200 (2 rows) @@ -652,10 +652,10 @@ s2: WARNING: upid: text savings = text checking: f step wnested2: <... completed> step c2: COMMIT; step read: SELECT * FROM accounts ORDER BY accountid; -accountid|balance ----------+------- -checking | 400 -savings | 600 +accountid|balance|balance2 +---------+-------+-------- +checking | 400| 800 +savings | 600| 1200 (2 rows) @@ -669,17 +669,17 @@ balance step updwcte: WITH doup AS (UPDATE accounts SET balance = balance + 1100 WHERE accountid = 'checking' RETURNING *) UPDATE accounts a SET balance = doup.balance + 100 FROM doup RETURNING *; step c1: COMMIT; step updwcte: <... completed> -accountid|balance|accountid|balance ----------+-------+---------+------- -savings | 1600|checking | 1500 +accountid|balance|balance2|accountid|balance|balance2 +---------+-------+--------+---------+-------+-------- +savings | 1600| 3200|checking | 1500| 3000 (1 row) step c2: COMMIT; step read: SELECT * FROM accounts ORDER BY accountid; -accountid|balance ----------+------- -checking | 1500 -savings | 1600 +accountid|balance|balance2 +---------+-------+-------- +checking | 1500| 3000 +savings | 1600| 3200 (2 rows) @@ -696,10 +696,10 @@ step updwctefail: <... completed> ERROR: tuple to be updated was already modified by an operation triggered by the current command step c2: COMMIT; step read: SELECT * FROM accounts ORDER BY accountid; -accountid|balance ----------+------- -checking | 400 -savings | 600 +accountid|balance|balance2 +---------+-------+-------- +checking | 400| 800 +savings | 600| 1200 (2 rows) @@ -713,16 +713,16 @@ balance step delwcte: WITH doup AS (UPDATE accounts SET balance = balance + 1100 WHERE accountid = 'checking' RETURNING *) DELETE FROM accounts a USING doup RETURNING *; step c1: COMMIT; step delwcte: <... completed> -accountid|balance|accountid|balance ----------+-------+---------+------- -savings | 600|checking | 1500 +accountid|balance|balance2|accountid|balance|balance2 +---------+-------+--------+---------+-------+-------- +savings | 600| 1200|checking | 1500| 3000 (1 row) step c2: COMMIT; step read: SELECT * FROM accounts ORDER BY accountid; -accountid|balance ----------+------- -checking | 1500 +accountid|balance|balance2 +---------+-------+-------- +checking | 1500| 3000 (1 row) @@ -739,10 +739,10 @@ step delwctefail: <... completed> ERROR: tuple to be deleted was already modified by an operation triggered by the current command step c2: COMMIT; step read: SELECT * FROM accounts ORDER BY accountid; -accountid|balance ----------+------- -checking | 400 -savings | 600 +accountid|balance|balance2 +---------+-------+-------- +checking | 400| 800 +savings | 600| 1200 (2 rows) @@ -767,10 +767,10 @@ step c1: COMMIT; step upsert2: <... completed> step c2: COMMIT; step read: SELECT * FROM accounts ORDER BY accountid; -accountid|balance ----------+------- -checking | 600 -savings | 2334 +accountid|balance|balance2 +---------+-------+-------- +checking | 600| 1200 +savings | 2334| 4668 (2 rows) @@ -856,18 +856,18 @@ step partiallock: step c2: COMMIT; step partiallock: <... completed> -accountid|balance|accountid|balance ----------+-------+---------+------- -checking | 1050|checking | 600 -savings | 600|savings | 600 +accountid|balance|balance2|accountid|balance|balance2 +---------+-------+--------+---------+-------+-------- +checking | 1050| 2100|checking | 600| 1200 +savings | 600| 1200|savings | 600| 1200 (2 rows) step c1: COMMIT; step read: SELECT * FROM accounts ORDER BY accountid; -accountid|balance ----------+------- -checking | 1050 -savings | 600 +accountid|balance|balance2 +---------+-------+-------- +checking | 1050| 2100 +savings | 600| 1200 (2 rows) @@ -887,18 +887,18 @@ step lockwithvalues: step c2: COMMIT; step lockwithvalues: <... completed> -accountid|balance|id ----------+-------+-------- -checking | 1050|checking -savings | 600|savings +accountid|balance|balance2|id +---------+-------+--------+-------- +checking | 1050| 2100|checking +savings | 600| 1200|savings (2 rows) step c1: COMMIT; step read: SELECT * FROM accounts ORDER BY accountid; -accountid|balance ----------+------- -checking | 1050 -savings | 600 +accountid|balance|balance2 +---------+-------+-------- +checking | 1050| 2100 +savings | 600| 1200 (2 rows) @@ -1104,25 +1104,31 @@ subid|id (1 row) -starting permutation: simplepartupdate complexpartupdate c1 c2 +starting permutation: simplepartupdate complexpartupdate c1 c2 read_part step simplepartupdate: - update parttbl set a = a; + update parttbl set b = b + 10; step complexpartupdate: - with u as (update parttbl set a = a returning parttbl.*) - update parttbl set a = u.a from u; + with u as (update parttbl set b = b + 1 returning parttbl.*) + update parttbl set b = u.b + 100 from u; step c1: COMMIT; step complexpartupdate: <... completed> step c2: COMMIT; +step read_part: SELECT * FROM parttbl ORDER BY a; +a| b|c| d +-+--+-+-- +1|12|1|13 +(1 row) -starting permutation: simplepartupdate_route1to2 complexpartupdate_route_err1 c1 c2 + +starting permutation: simplepartupdate_route1to2 complexpartupdate_route_err1 c1 c2 read_part step simplepartupdate_route1to2: update parttbl set a = 2 where c = 1 returning *; -a|b|c --+-+- -2|1|1 +a|b|c|d +-+-+-+- +2|1|1|3 (1 row) step complexpartupdate_route_err1: @@ -1133,14 +1139,20 @@ 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; +a|b|c|d +-+-+-+- +2|1|1|3 +(1 row) -starting permutation: simplepartupdate_noroute complexpartupdate_route c1 c2 + +starting permutation: simplepartupdate_noroute complexpartupdate_route c1 c2 read_part step simplepartupdate_noroute: update parttbl set b = 2 where c = 1 returning *; -a|b|c --+-+- -1|2|1 +a|b|c|d +-+-+-+- +1|2|1|3 (1 row) step complexpartupdate_route: @@ -1149,20 +1161,26 @@ step complexpartupdate_route: step c1: COMMIT; step complexpartupdate_route: <... completed> -a|b|c --+-+- -2|2|1 +a|b|c|d +-+-+-+- +2|2|1|4 (1 row) step c2: COMMIT; +step read_part: SELECT * FROM parttbl ORDER BY a; +a|b|c|d +-+-+-+- +2|2|1|4 +(1 row) -starting permutation: simplepartupdate_noroute complexpartupdate_doesnt_route c1 c2 + +starting permutation: simplepartupdate_noroute complexpartupdate_doesnt_route c1 c2 read_part step simplepartupdate_noroute: update parttbl set b = 2 where c = 1 returning *; -a|b|c --+-+- -1|2|1 +a|b|c|d +-+-+-+- +1|2|1|3 (1 row) step complexpartupdate_doesnt_route: @@ -1171,9 +1189,15 @@ step complexpartupdate_doesnt_route: step c1: COMMIT; step complexpartupdate_doesnt_route: <... completed> -a|b|c --+-+- -1|2|1 +a|b|c|d +-+-+-+- +1|2|1|3 (1 row) step c2: COMMIT; +step read_part: SELECT * FROM parttbl ORDER BY a; +a|b|c|d +-+-+-+- +1|2|1|3 +(1 row) + diff --git a/src/test/isolation/specs/eval-plan-qual.spec b/src/test/isolation/specs/eval-plan-qual.spec index 8ed1ecc3a4..f9bbe2d9f0 100644 --- a/src/test/isolation/specs/eval-plan-qual.spec +++ b/src/test/isolation/specs/eval-plan-qual.spec @@ -6,7 +6,8 @@ setup { - CREATE TABLE accounts (accountid text PRIMARY KEY, balance numeric not null); + CREATE TABLE accounts (accountid text PRIMARY KEY, balance numeric not null, + balance2 numeric GENERATED ALWAYS AS (balance * 2) STORED); INSERT INTO accounts VALUES ('checking', 600), ('savings', 600); CREATE FUNCTION update_checking(int) RETURNS bool LANGUAGE sql AS $$ @@ -33,7 +34,8 @@ setup CREATE TABLE jointest AS SELECT generate_series(1,10) AS id, 0 AS data; CREATE INDEX ON jointest(id); - CREATE TABLE parttbl (a int, b int, c int) PARTITION BY LIST (a); + CREATE TABLE parttbl (a int, b int, c int, + 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); @@ -171,7 +173,7 @@ step selectresultforupdate { # test for EPQ on a partitioned result table step simplepartupdate { - update parttbl set a = a; + update parttbl set b = b + 10; } # test scenarios where update may cause row movement @@ -223,8 +225,8 @@ step updateforcip3 { step wrtwcte { UPDATE table_a SET value = 'tableAValue2' WHERE id = 1; } step wrjt { UPDATE jointest SET data = 42 WHERE id = 7; } step complexpartupdate { - with u as (update parttbl set a = a returning parttbl.*) - update parttbl set a = u.a from u; + with u as (update parttbl set b = b + 1 returning parttbl.*) + update parttbl set b = u.b + 100 from u; } step complexpartupdate_route_err1 { @@ -273,6 +275,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; } # this test exercises EvalPlanQual with a CTE, cf bug #14328 step readwcte { @@ -353,7 +356,7 @@ permutation wrjt selectjoinforupdate c2 c1 permutation wrjt selectresultforupdate c2 c1 permutation wrtwcte multireadwcte c1 c2 -permutation simplepartupdate complexpartupdate c1 c2 -permutation simplepartupdate_route1to2 complexpartupdate_route_err1 c1 c2 -permutation simplepartupdate_noroute complexpartupdate_route c1 c2 -permutation simplepartupdate_noroute complexpartupdate_doesnt_route c1 c2 +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 +permutation simplepartupdate_noroute complexpartupdate_doesnt_route c1 c2 read_part diff --git a/src/test/regress/expected/generated.out b/src/test/regress/expected/generated.out index 9eec3f3716..2d4285d1dc 100644 --- a/src/test/regress/expected/generated.out +++ b/src/test/regress/expected/generated.out @@ -637,6 +637,7 @@ DROP TABLE gtest_parent; -- partitioned table CREATE TABLE gtest_parent (f1 date NOT NULL, f2 bigint, f3 bigint GENERATED ALWAYS AS (f2 * 2) STORED) PARTITION BY RANGE (f1); CREATE TABLE gtest_child PARTITION OF gtest_parent FOR VALUES FROM ('2016-07-01') TO ('2016-08-01'); +CREATE TABLE gtest_child3 PARTITION OF gtest_parent FOR VALUES FROM ('2016-09-01') TO ('2016-10-01'); INSERT INTO gtest_parent (f1, f2) VALUES ('2016-07-15', 1); SELECT * FROM gtest_parent; f1 | f2 | f3 @@ -650,6 +651,19 @@ SELECT * FROM gtest_child; 07-15-2016 | 1 | 2 (1 row) +UPDATE gtest_parent SET f1 = f1 + 60, f2 = f2 + 1; +SELECT * FROM gtest_parent; + f1 | f2 | f3 +------------+----+---- + 09-13-2016 | 2 | 4 +(1 row) + +SELECT * FROM gtest_child3; + f1 | f2 | f3 +------------+----+---- + 09-13-2016 | 2 | 4 +(1 row) + DROP TABLE gtest_parent; -- generated columns in partition key (not allowed) CREATE TABLE gtest_parent (f1 date NOT NULL, f2 bigint, f3 bigint GENERATED ALWAYS AS (f2 * 2) STORED) PARTITION BY RANGE (f3); diff --git a/src/test/regress/sql/generated.sql b/src/test/regress/sql/generated.sql index 9bc4f76606..247cb900c2 100644 --- a/src/test/regress/sql/generated.sql +++ b/src/test/regress/sql/generated.sql @@ -347,9 +347,13 @@ DROP TABLE gtest_parent; -- partitioned table CREATE TABLE gtest_parent (f1 date NOT NULL, f2 bigint, f3 bigint GENERATED ALWAYS AS (f2 * 2) STORED) PARTITION BY RANGE (f1); CREATE TABLE gtest_child PARTITION OF gtest_parent FOR VALUES FROM ('2016-07-01') TO ('2016-08-01'); +CREATE TABLE gtest_child3 PARTITION OF gtest_parent FOR VALUES FROM ('2016-09-01') TO ('2016-10-01'); INSERT INTO gtest_parent (f1, f2) VALUES ('2016-07-15', 1); SELECT * FROM gtest_parent; SELECT * FROM gtest_child; +UPDATE gtest_parent SET f1 = f1 + 60, f2 = f2 + 1; +SELECT * FROM gtest_parent; +SELECT * FROM gtest_child3; DROP TABLE gtest_parent; -- generated columns in partition key (not allowed)