Fix misbehavior of CTE-used-in-a-subplan during EPQ rechecks.

An updating query that reads a CTE within an InitPlan or SubPlan could get
incorrect results if it updates rows that are concurrently being modified.
This is caused by CteScanNext supposing that nothing inside its recursive
ExecProcNode call could change which read pointer is selected in the CTE's
shared tuplestore.  While that's normally true because of scoping
considerations, it can break down if an EPQ plan tree gets built during the
call, because EvalPlanQualStart builds execution trees for all subplans
whether they're going to be used during the recheck or not.  And it seems
like a pretty shaky assumption anyway, so let's just reselect our own read
pointer here.

Per bug #14870 from Andrei Gorita.  This has been broken since CTEs were
implemented, so back-patch to all supported branches.

Discussion: https://postgr.es/m/20171024155358.1471.82377@wrigleys.postgresql.org

diff --git a/src/backend/executor/nodeCtescan.c b/src/backend/executor/nodeCtescan.c
index 9ea318ae7da5531961f5d9e57aa757da16aac2d5..ae7bfce4e5eea12d00e965f2857da1d7320f9067 100644 (file)
--- a/src/backend/executor/nodeCtescan.c
+++ b/src/backend/executor/nodeCtescan.c
@@ -107,6 +107,13 @@ CteScanNext(CteScanState *node)
                        return NULL;
                }
 
+               /*
+                * There are corner cases where the subplan could change which
+                * tuplestore read pointer is active, so be sure to reselect ours
+                * before storing the tuple we got.
+                */
+               tuplestore_select_read_pointer(tuplestorestate, node->readptr);
+
                /*
                 * Append a copy of the returned tuple to tuplestore.  NOTE: because
                 * our read pointer is certainly in EOF state, its read position will
@@ -176,6 +183,12 @@ ExecInitCteScan(CteScan *node, EState *estate, int eflags)
         * we might be asked to rescan the CTE even though upper levels didn't
         * tell us to be prepared to do it efficiently.  Annoying, since this
         * prevents truncation of the tuplestore.  XXX FIXME
+        *
+        * Note: if we are in an EPQ recheck plan tree, it's likely that no access
+        * to the tuplestore is needed at all, making this even more annoying.
+        * It's not worth improving that as long as all the read pointers would
+        * have REWIND anyway, but if we ever improve this logic then that aspect
+        * should be considered too.
         */
        eflags |= EXEC_FLAG_REWIND;
 

diff --git a/src/test/isolation/expected/eval-plan-qual.out b/src/test/isolation/expected/eval-plan-qual.out
index 99c4137e14a5b7d1179308002b412a21dae05b38..be87d1b0c2760b9e322cffb71819d08fec88ecdb 100644 (file)
--- a/src/test/isolation/expected/eval-plan-qual.out
+++ b/src/test/isolation/expected/eval-plan-qual.out
@@ -125,3 +125,18 @@ step readwcte: <... completed>
 id             value          
 
 1              tableAValue2   
+
+starting permutation: wrtwcte multireadwcte c1 c2
+step wrtwcte: UPDATE table_a SET value = 'tableAValue2' WHERE id = 1;
+step multireadwcte: 
+       WITH updated AS (
+         UPDATE table_a SET value = 'tableAValue3' WHERE id = 1 RETURNING id
+       )
+       SELECT (SELECT id FROM updated) AS subid, * FROM updated;
+ <waiting ...>
+step c1: COMMIT;
+step c2: COMMIT;
+step multireadwcte: <... completed>
+subid          id             
+
+1              1              

diff --git a/src/test/isolation/specs/eval-plan-qual.spec b/src/test/isolation/specs/eval-plan-qual.spec
index f15dee385fce681dbe4eed8231088e1782895832..749c64057960b51f3b532b0ede4c5afb304a5a77 100644 (file)
--- a/src/test/isolation/specs/eval-plan-qual.spec
+++ b/src/test/isolation/specs/eval-plan-qual.spec
@@ -94,6 +94,14 @@ step "readwcte"      {
        SELECT * FROM cte2;
 }
 
+# this test exercises a different CTE misbehavior, cf bug #14870
+step "multireadwcte"   {
+       WITH updated AS (
+         UPDATE table_a SET value = 'tableAValue3' WHERE id = 1 RETURNING id
+       )
+       SELECT (SELECT id FROM updated) AS subid, * FROM updated;
+}
+
 teardown       { COMMIT; }
 
 permutation "wx1" "wx2" "c1" "c2" "read"
@@ -102,3 +110,4 @@ permutation "upsert1" "upsert2" "c1" "c2" "read"
 permutation "readp1" "writep1" "readp2" "c1" "c2"
 permutation "writep2" "returningp1" "c1" "c2"
 permutation "wrtwcte" "readwcte" "c1" "c2"
+permutation "wrtwcte" "multireadwcte" "c1" "c2"