Thread: BUG #15336: Wrong cursor's bacward fetch results in select withALL(subquery)

The following bug has been logged on the website:

Bug reference:      15336
Logged by:          Vladimir Baranoff
Email address:      v.g.baranoff@gmail.com
PostgreSQL version: 10.0
Operating system:   Ubuntu 18.04
Description:

create table longs (value int8 ) with ( oids=false );
insert into longs (value) values (1), (2), (3), (4), (5), (6), (7), (8),
(9);

Now fetch results with the cursor in forward direction:

begin;
declare "mycursor" binary scroll cursor for
  select "l".value as "column1"
    from longs as "l"
   where "l".value <> all( select 5 union all select 6 union all select 7)
;
move absolute 0 in "mycursor";
fetch forward 9 from "mycursor";
commit;
The result set is correct (1, 2, 3, 4, 8, 9).

Then execute the following script:

begin;
declare "mycursor" binary scroll cursor for
  select "l".value as "column1"
    from longs as "l"
   where "l".value <> all ( select 5 union all select 6 union all select 7
)
;
move absolute 10 in "mycursor";
fetch backward 9 from "mycursor";
commit;
The result set is wrong (9, 8, 7, 6, 5, 4, 3, 2, 1).
It seems that selection predicate is ignored.
Replacing ALL(subquery) by NOT IN (subquery) solves the problem, but this
violates statement that "NOT IN is equivalent to <> ALL.".
This bug has been reproduced with PostgreSQL 9.6 and 10.0

>>>>> "PG" == PG Bug reporting form <noreply@postgresql.org> writes:

 PG> fetch backward 9 from "mycursor";
 PG> commit;
 PG> The result set is wrong (9, 8, 7, 6, 5, 4, 3, 2, 1).
 PG> It seems that selection predicate is ignored.

 PG> This bug has been reproduced with PostgreSQL 9.6 and 10.0

I wonder if we have a contender here for the oldest reported bug in PG
history; while I haven't tested anything older than 9.5, the incorrect
logic seems to date back to the introduction of subqueries in
6.something.

Here is a simpler test case:

begin;
declare foo cursor for select * from generate_series(1,3) i where i <> all (values (2));
fetch all from foo;  -- returns the expected 2 rows
fetch backward all from foo;  -- assertion failure, or incorrect result

The problem is that the scan direction is being set to "backward" in the
EState, and as a result the subquery evaluation is run in the backward
direction too, which obviously doesn't do anything sensible. The
assertion failure is from the tuplestore code complaining about doing a
backward fetch on a tuplestore not initialized for backward access.

I'm really not sure yet what the correct fix is, though.

-- 
Andrew (irc:RhodiumToad)

>>>>> "Andrew" == Andrew Gierth <andrew@tao11.riddles.org.uk> writes:

 Andrew> I'm really not sure yet what the correct fix is, though.

I'm guessing that locally saving/restoring the scan direction in
ExecSubPlan is going to be the best option; it seems to fix the problem,
I'll post a patch in a bit.

-- 
Andrew (irc:RhodiumToad)

>>>>> "Andrew" == Andrew Gierth <andrew@tao11.riddles.org.uk> writes:

 Andrew> I'm guessing that locally saving/restoring the scan direction
 Andrew> in ExecSubPlan is going to be the best option; it seems to fix
 Andrew> the problem, I'll post a patch in a bit.

It turns out to be also necessary to do this in ExecSetParamPlan, though
I couldn't find a way to make a stable regression test for that - my
manual tests were based on putting a subselect inside a volatile
construct like CASE WHEN random() < x THEN.

Patch attached.

-- 
Andrew (irc:RhodiumToad)

diff --git a/src/backend/executor/nodeSubplan.c b/src/backend/executor/nodeSubplan.c
index 44f551bcf1..6b370750c5 100644
--- a/src/backend/executor/nodeSubplan.c
+++ b/src/backend/executor/nodeSubplan.c
@@ -65,6 +65,9 @@ ExecSubPlan(SubPlanState *node,
             bool *isNull)
 {
     SubPlan    *subplan = node->subplan;
+    EState       *estate = node->planstate->state;
+    ScanDirection dir = estate->es_direction;
+    Datum        retval;
 
     CHECK_FOR_INTERRUPTS();
 
@@ -77,11 +80,19 @@ ExecSubPlan(SubPlanState *node,
     if (subplan->setParam != NIL && subplan->subLinkType != MULTIEXPR_SUBLINK)
         elog(ERROR, "cannot set parent params from subquery");
 
+    /* Force forward-scan mode for evaluation */
+    estate->es_direction = ForwardScanDirection;
+
     /* Select appropriate evaluation strategy */
     if (subplan->useHashTable)
-        return ExecHashSubPlan(node, econtext, isNull);
+        retval = ExecHashSubPlan(node, econtext, isNull);
     else
-        return ExecScanSubPlan(node, econtext, isNull);
+        retval = ExecScanSubPlan(node, econtext, isNull);
+
+    /* restore scan direction */
+    estate->es_direction = dir;
+
+    return retval;
 }
 
 /*
@@ -1006,6 +1017,8 @@ ExecSetParamPlan(SubPlanState *node, ExprContext *econtext)
     SubPlan    *subplan = node->subplan;
     PlanState  *planstate = node->planstate;
     SubLinkType subLinkType = subplan->subLinkType;
+    EState       *estate = planstate->state;
+    ScanDirection dir = estate->es_direction;
     MemoryContext oldcontext;
     TupleTableSlot *slot;
     ListCell   *pvar;
@@ -1019,6 +1032,12 @@ ExecSetParamPlan(SubPlanState *node, ExprContext *econtext)
     if (subLinkType == CTE_SUBLINK)
         elog(ERROR, "CTE subplans should not be executed via ExecSetParamPlan");
 
+    /*
+     * Enforce forward scan direction regardless of caller. It's hard but not
+     * impossible to get here in backward scan, so make it work anyway.
+     */
+    estate->es_direction = ForwardScanDirection;
+
     /* Initialize ArrayBuildStateAny in caller's context, if needed */
     if (subLinkType == ARRAY_SUBLINK)
         astate = initArrayResultAny(subplan->firstColType,
@@ -1171,6 +1190,9 @@ ExecSetParamPlan(SubPlanState *node, ExprContext *econtext)
     }
 
     MemoryContextSwitchTo(oldcontext);
+
+    /* restore scan direction */
+    estate->es_direction = dir;
 }
 
 /*
diff --git a/src/test/regress/expected/subselect.out b/src/test/regress/expected/subselect.out
index 2904ae43e5..588d069589 100644
--- a/src/test/regress/expected/subselect.out
+++ b/src/test/regress/expected/subselect.out
@@ -1137,3 +1137,20 @@ select * from (select pk,c2 from sq_limit order by c1,pk) as x limit 3;
 
 drop function explain_sq_limit();
 drop table sq_limit;
+--
+-- Ensure that backward scan direction isn't propagated into
+-- expression subqueries (bug #15336)
+--
+begin;
+declare c1 scroll cursor for
+ select * from generate_series(1,4) i
+  where i <> all (values (2),(3));
+move forward all in c1;
+fetch backward all in c1;
+ i 
+---
+ 4
+ 1
+(2 rows)
+
+commit;
diff --git a/src/test/regress/sql/subselect.sql b/src/test/regress/sql/subselect.sql
index 9b7125c111..843f511b3d 100644
--- a/src/test/regress/sql/subselect.sql
+++ b/src/test/regress/sql/subselect.sql
@@ -609,3 +609,19 @@ select * from (select pk,c2 from sq_limit order by c1,pk) as x limit 3;
 drop function explain_sq_limit();
 
 drop table sq_limit;
+
+--
+-- Ensure that backward scan direction isn't propagated into
+-- expression subqueries (bug #15336)
+--
+
+begin;
+
+declare c1 scroll cursor for
+ select * from generate_series(1,4) i
+  where i <> all (values (2),(3));
+
+move forward all in c1;
+fetch backward all in c1;
+
+commit;

Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> "Andrew" == Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
>  Andrew> I'm guessing that locally saving/restoring the scan direction
>  Andrew> in ExecSubPlan is going to be the best option; it seems to fix
>  Andrew> the problem, I'll post a patch in a bit.

> It turns out to be also necessary to do this in ExecSetParamPlan, though
> I couldn't find a way to make a stable regression test for that - my
> manual tests were based on putting a subselect inside a volatile
> construct like CASE WHEN random() < x THEN.

Looks sane to me ... and a bit astonishing that we didn't run into
this a decade or two back.

            regards, tom lane

On 2018-Aug-17, Andrew Gierth wrote:

> I wonder if we have a contender here for the oldest reported bug in PG
> history; while I haven't tested anything older than 9.5, the incorrect
> logic seems to date back to the introduction of subqueries in
> 6.something.

Hmm ..

> begin;
> declare foo cursor for select * from generate_series(1,3) i where i <> all (values (2));
> fetch all from foo;  -- returns the expected 2 rows
> fetch backward all from foo;  -- assertion failure, or incorrect result

8.2 seems fine:

alvherre=# show debug_assertions;
 debug_assertions 
------------------
 on
(1 fila)

alvherre=# begin;
BEGIN
alvherre=# declare foo cursor for select * from generate_series(1,3) i where i <> all (values (2));
DECLARE CURSOR
alvherre=# fetch all from foo;
 i 
---
 1
 3
(2 filas)
alvherre=# fetch backward all from foo;
 i 
---
 3
 1
(2 filas)

9.1 does fail an assertion:

TRAP: FailedAssertion(«!(forward || (readptr->eflags & 0x0004))», Archivo:
«/pgsql/source/REL9_1_STABLE/src/backend/utils/sort/tuplestore.c»,Línea: 765)
 

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

>>>>> "Alvaro" == Alvaro Herrera <alvherre@2ndquadrant.com> writes:

 >> I wonder if we have a contender here for the oldest reported bug in
 >> PG history; while I haven't tested anything older than 9.5, the
 >> incorrect logic seems to date back to the introduction of subqueries
 >> in 6.something.

 Alvaro> Hmm ..

 Alvaro> 8.2 seems fine:

Hah. You're right; the bug is only 10 years old, not 20. It was
apparently introduced in 8.3 by commit c7ff7663e; before that, SubPlans
had a separate EState from the parent plan, so they didn't share the
parent plan's direction indicator.

-- 
Andrew (irc:RhodiumToad)

>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:

 >> It turns out to be also necessary to do this in ExecSetParamPlan,
 >> though I couldn't find a way to make a stable regression test for
 >> that - my manual tests were based on putting a subselect inside a
 >> volatile construct like CASE WHEN random() < x THEN.

 Tom> Looks sane to me ...

Pushed.

-- 
Andrew (irc:RhodiumToad)