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.

diff --git a/src/backend/executor/nodeLockRows.c b/src/backend/executor/nodeLockRows.c
index 814b61efcbaea189cbccf0eed46f3f18a2d89f7b..5131788f24edb5adc362e257829841f60a7c57dd 100644 (file)
--- a/src/backend/executor/nodeLockRows.c
+++ b/src/backend/executor/nodeLockRows.c
@@ -187,7 +187,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 ab778cbd7aaec646570df8ce55681ee3e7dc5f2b..0f6595fcb1b63f0ffdc2a2d9c0eb9083fa12b3f2 100644 (file)
--- 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; <waiting ...>
+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 786b7913652f0b5351e28ad8492dc3e94c1bf899..876e5470dba5dfeb109f37cc567aa10572e3da9b 100644 (file)
--- 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"