From 2db576ba8c449fcaf61ae7aa14ed62e63ebf5924 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Thu, 11 Dec 2014 19:37:00 -0500 Subject: [PATCH] Fix corner case where SELECT FOR UPDATE could return a row twice. In READ COMMITTED mode, if a SELECT FOR UPDATE discovers it has to redo WHERE-clause checking on rows that have been updated since the SELECT's snapshot, it invokes EvalPlanQual processing to do that. If this first occurs within a non-first child table of an inheritance tree, the previous coding could accidentally re-return a matching row from an earlier, already-scanned child table. (And, to add insult to injury, I think this could make it miss returning a row that should have been returned, if the updated row that this happens on should still have passed the WHERE qual.) Per report from Kyotaro Horiguchi; the added isolation test is based on his test case. This has been broken for quite awhile, so back-patch to all supported branches. --- src/backend/executor/nodeLockRows.c | 22 ++++++++++++++++++ .../isolation/expected/eval-plan-qual.out | 23 +++++++++++++++++++ src/test/isolation/specs/eval-plan-qual.spec | 16 +++++++++++++ 3 files changed, 61 insertions(+) diff --git a/src/backend/executor/nodeLockRows.c b/src/backend/executor/nodeLockRows.c index 990240bf0a..5dcb9b042a 100644 --- a/src/backend/executor/nodeLockRows.c +++ b/src/backend/executor/nodeLockRows.c @@ -194,7 +194,29 @@ lnext: */ if (!epq_started) { + ListCell *lc2; + EvalPlanQualBegin(&node->lr_epqstate, estate); + + /* + * Ensure that rels with already-visited rowmarks are told + * not to return tuples during the first EPQ test. We can + * exit this loop once it reaches the current rowmark; + * rels appearing later in the list will be set up + * correctly by the EvalPlanQualSetTuple call at the top + * of the loop. + */ + foreach(lc2, node->lr_arowMarks) + { + ExecAuxRowMark *aerm2 = (ExecAuxRowMark *) lfirst(lc2); + + if (lc2 == lc) + break; + EvalPlanQualSetTuple(&node->lr_epqstate, + aerm2->rowmark->rti, + NULL); + } + epq_started = true; } diff --git a/src/test/isolation/expected/eval-plan-qual.out b/src/test/isolation/expected/eval-plan-qual.out index ab778cbd7a..0f6595fcb1 100644 --- a/src/test/isolation/expected/eval-plan-qual.out +++ b/src/test/isolation/expected/eval-plan-qual.out @@ -49,3 +49,26 @@ accountid balance checking 600 savings 2334 + +starting permutation: readp1 writep1 readp2 c1 c2 +step readp1: SELECT tableoid::regclass, ctid, * FROM p WHERE b IN (0, 1) AND c = 0 FOR UPDATE; +tableoid ctid a b c + +c1 (0,1) 0 0 0 +c1 (0,4) 0 1 0 +c2 (0,1) 1 0 0 +c2 (0,4) 1 1 0 +c3 (0,1) 2 0 0 +c3 (0,4) 2 1 0 +step writep1: UPDATE p SET b = -1 WHERE a = 1 AND b = 1 AND c = 0; +step readp2: SELECT tableoid::regclass, ctid, * FROM p WHERE b IN (0, 1) AND c = 0 FOR UPDATE; +step c1: COMMIT; +step readp2: <... completed> +tableoid ctid a b c + +c1 (0,1) 0 0 0 +c1 (0,4) 0 1 0 +c2 (0,1) 1 0 0 +c3 (0,1) 2 0 0 +c3 (0,4) 2 1 0 +step c2: COMMIT; diff --git a/src/test/isolation/specs/eval-plan-qual.spec b/src/test/isolation/specs/eval-plan-qual.spec index 786b791365..876e5470db 100644 --- a/src/test/isolation/specs/eval-plan-qual.spec +++ b/src/test/isolation/specs/eval-plan-qual.spec @@ -8,11 +8,20 @@ setup { CREATE TABLE accounts (accountid text PRIMARY KEY, balance numeric not null); INSERT INTO accounts VALUES ('checking', 600), ('savings', 600); + + CREATE TABLE p (a int, b int, c int); + CREATE TABLE c1 () INHERITS (p); + CREATE TABLE c2 () INHERITS (p); + CREATE TABLE c3 () INHERITS (p); + INSERT INTO c1 SELECT 0, a / 3, a % 3 FROM generate_series(0, 9) a; + INSERT INTO c2 SELECT 1, a / 3, a % 3 FROM generate_series(0, 9) a; + INSERT INTO c3 SELECT 2, a / 3, a % 3 FROM generate_series(0, 9) a; } teardown { DROP TABLE accounts; + DROP TABLE p CASCADE; } session "s1" @@ -30,6 +39,11 @@ step "upsert1" { INSERT INTO accounts SELECT 'savings', 500 WHERE NOT EXISTS (SELECT 1 FROM upsert); } +# tests with table p check inheritance cases, specifically a bug where +# nodeLockRows did the wrong thing when the first updated tuple was in +# a non-first child table +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 "c1" { COMMIT; } session "s2" @@ -44,6 +58,7 @@ step "upsert2" { INSERT INTO accounts SELECT 'savings', 1234 WHERE NOT EXISTS (SELECT 1 FROM upsert); } +step "readp2" { SELECT tableoid::regclass, ctid, * FROM p WHERE b IN (0, 1) AND c = 0 FOR UPDATE; } step "c2" { COMMIT; } session "s3" @@ -54,3 +69,4 @@ teardown { COMMIT; } permutation "wx1" "wx2" "c1" "c2" "read" permutation "wy1" "wy2" "c1" "c2" "read" permutation "upsert1" "upsert2" "c1" "c2" "read" +permutation "readp1" "writep1" "readp2" "c1" "c2"