Re: [Bug] Duplicate results for inheritance and FOR UPDATE. - Mailing list pgsql-hackers

Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> writes:
> This issue is that some query returns duplicate rows after FOR
> UPDATE was blocked, in other words, after getting
> HeapTupleUpdated in ExecLockRows.

Good catch!

> In the EPQ block in ExecScanFetch, it seems should return NULL if
> the relation does not has test tuple. The attached patch does so
> on the current master and the problem has disappeared.

... but bad fix.  This would break join cases, wherein it's important
that other scan nodes still return all the required tuples.  (It's
unfortunate that the eval-plan-qual isolation test fails to cover
joins :-(.)

After digging around a bit, I realized that the problem is actually in
nodeLockRows.c.  It is supposed to do EvalPlanQualSetTuple(..., NULL)
for each child table that's not supposed to return any row during the
current EPQ test cycle.  Unfortunately, it only does that reliably once
the EPQ environment is already set up.  If we discover we need an EPQ
test while looking at a non-first child table, tables already passed
over in the loop over node->lr_arowMarks don't get the word.

So the correct fix (or a correct fix, anyway; this could also have been
done with more-invasive loop logic changes) is as attached.  I'm working
on back-patching this.

            regards, tom lane

diff --git a/src/backend/executor/nodeLockRows.c b/src/backend/executor/nodeLockRows.c
index 990240b..5dcb9b0 100644
*** a/src/backend/executor/nodeLockRows.c
--- b/src/backend/executor/nodeLockRows.c
*************** lnext:
*** 194,200 ****
--- 194,222 ----
                   */
                  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 ab778cb..0f6595f 100644
*** a/src/test/isolation/expected/eval-plan-qual.out
--- b/src/test/isolation/expected/eval-plan-qual.out
*************** accountid      balance
*** 49,51 ****
--- 49,74 ----

  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 786b791..876e547 100644
*** a/src/test/isolation/specs/eval-plan-qual.spec
--- b/src/test/isolation/specs/eval-plan-qual.spec
*************** setup
*** 8,18 ****
--- 8,27 ----
  {
   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"
*************** step "upsert1"    {
*** 30,35 ****
--- 39,49 ----
      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"
*************** step "upsert2"    {
*** 44,49 ****
--- 58,64 ----
      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"
*************** teardown    { COMMIT; }
*** 54,56 ****
--- 69,72 ----
  permutation "wx1" "wx2" "c1" "c2" "read"
  permutation "wy1" "wy2" "c1" "c2" "read"
  permutation "upsert1" "upsert2" "c1" "c2" "read"
+ permutation "readp1" "writep1" "readp2" "c1" "c2"