Re: We don't enforce NO SCROLL cursor restrictions - Mailing list pgsql-hackers

I wrote:
> The reason for this behavior is that the only-scan-forward check
> is in the relatively low-level function PortalRunSelect, which
> is passed a "forward" flag and a count.  The place that interprets
> FETCH_ABSOLUTE and friends is DoPortalRunFetch, and what it's doing
> in this particular scenario is to rewind to start and fetch forwards,
> thus bypassing PortalRunSelect's error check.

After some further study, I've reached a few conclusions:

* The missing bit in pquery.c is exactly that we'll allow a portal
rewind even with a no-scroll cursor.  I think that the reason it's
like that is that the code was mainly interested in closing off
cases where we'd attempt to run a plan backwards, to protect plan
node types that can't do that.  As far as the executor is concerned,
rewind-to-start is okay in any case.  However, as we see from this
thread, that definition doesn't protect us against anomalous results
from volatile queries.  So putting an error check in DoPortalRewind
seems to be enough to fix this, as in patch 0001 below.  (This also
fixes one bogus copied-and-pasted comment, and adjusts the one
regression test case that breaks.)

* The anomaly for held cursors boils down to ba2c6d6ce having ignored
this good advice in portal.h:

     * ... Also note that various code inspects atStart and atEnd, but
     * only the portal movement routines should touch portalPos.

Thus, PersistHoldablePortal has no business changing the cursor's
atStart/atEnd/portalPos.  The only thing that resetting portalPos
actually bought us was to make the tuplestore_skiptuples call a bit
further down into a no-op, but we can just bypass that call for a
no-scroll cursor, as in 0002 below.  However, 0002 does have a
dependency on 0001, because if we allow tuplestore_rescan on the
holdStore it will expose the fact that the tuplestore doesn't contain
the whole cursor result.  (I was a bit surprised to find that those
were the only two places where we weren't positioning in the holdStore
by dead reckoning, but it seems to be the case.)

I was feeling nervous about back-patching 0001 already, and finding
that one of our own regression tests was dependent on the omission
of this check doesn't make me any more confident.  However, I'd really
like to be able to back-patch 0002 to get rid of the held-cursor
positioning anomaly.  What I think might be an acceptable compromise
in the back branches is to have DoPortalRewind complain only if
(a) it needs to reposition a no-scroll cursor AND (b) the cursor has
a holdStore, ie it's held over from some previous transaction.
The extra restriction (b) should prevent most people from running into
the error check, even if they've been sloppy about marking cursors
scrollable.  In HEAD we'd drop the restriction (b) and commit 0001 as
shown.  I'm kind of inclined to do that in v14 too, but there's an
argument to be made that it's too late in the beta process to be
changing user-visible semantics without great need.

Thoughts?

            regards, tom lane

diff --git a/src/backend/tcop/pquery.c b/src/backend/tcop/pquery.c
index a3c27d9d74..a9e1056057 100644
--- a/src/backend/tcop/pquery.c
+++ b/src/backend/tcop/pquery.c
@@ -1472,9 +1472,8 @@ PortalRunFetch(Portal portal,
  * DoPortalRunFetch
  *        Guts of PortalRunFetch --- the portal context is already set up
  *
- * count <= 0 is interpreted as a no-op: the destination gets started up
- * and shut down, but nothing else happens.  Also, count == FETCH_ALL is
- * interpreted as "all rows".  (cf FetchStmt.howMany)
+ * Here, count < 0 typically reverses the direction.  Also, count == FETCH_ALL
+ * is interpreted as "all rows".  (cf FetchStmt.howMany)
  *
  * Returns number of rows processed (suitable for use in result tag)
  */
@@ -1491,6 +1490,15 @@ DoPortalRunFetch(Portal portal,
            portal->strategy == PORTAL_ONE_MOD_WITH ||
            portal->strategy == PORTAL_UTIL_SELECT);

+    /*
+     * Note: we disallow backwards fetch (including re-fetch of current row)
+     * for NO SCROLL cursors, but we interpret that very loosely: you can use
+     * any of the FetchDirection options, so long as the end result is to move
+     * forwards by at least one row.  Currently it's sufficient to check for
+     * NO SCROLL in DoPortalRewind() and in the forward == false path in
+     * PortalRunSelect(); but someday we might prefer to account for that
+     * restriction explicitly here.
+     */
     switch (fdirection)
     {
         case FETCH_FORWARD:
@@ -1668,6 +1676,19 @@ DoPortalRewind(Portal portal)
 {
     QueryDesc  *queryDesc;

+    /*
+     * No work is needed if we've not advanced the cursor (and we don't want
+     * to throw a NO SCROLL error in this case).
+     */
+    if (portal->atStart && !portal->atEnd)
+        return;
+
+    if (portal->cursorOptions & CURSOR_OPT_NO_SCROLL)
+        ereport(ERROR,
+                (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+                 errmsg("cursor can only scan forward"),
+                 errhint("Declare it with SCROLL option to enable backward scan.")));
+
     /* Rewind holdStore, if we have one */
     if (portal->holdStore)
     {
diff --git a/src/test/regress/expected/portals.out b/src/test/regress/expected/portals.out
index 42dc637fd4..d98c58d303 100644
--- a/src/test/regress/expected/portals.out
+++ b/src/test/regress/expected/portals.out
@@ -715,6 +715,24 @@ FETCH BACKWARD 1 FROM foo24; -- should fail
 ERROR:  cursor can only scan forward
 HINT:  Declare it with SCROLL option to enable backward scan.
 END;
+BEGIN;
+DECLARE foo24 NO SCROLL CURSOR FOR SELECT * FROM tenk1 ORDER BY unique2;
+FETCH 1 FROM foo24;
+ unique1 | unique2 | two | four | ten | twenty | hundred | thousand | twothousand | fivethous | tenthous | odd | even
|stringu1 | stringu2 | string4  

+---------+---------+-----+------+-----+--------+---------+----------+-------------+-----------+----------+-----+------+----------+----------+---------
+    8800 |       0 |   0 |    0 |   0 |      0 |       0 |      800 |         800 |      3800 |     8800 |   0 |    1
|MAAAAA   | AAAAAA   | AAAAxx 
+(1 row)
+
+FETCH ABSOLUTE 2 FROM foo24; -- allowed
+ unique1 | unique2 | two | four | ten | twenty | hundred | thousand | twothousand | fivethous | tenthous | odd | even
|stringu1 | stringu2 | string4  

+---------+---------+-----+------+-----+--------+---------+----------+-------------+-----------+----------+-----+------+----------+----------+---------
+    1891 |       1 |   1 |    3 |   1 |     11 |      91 |      891 |        1891 |      1891 |     1891 | 182 |  183
|TUAAAA   | BAAAAA   | HHHHxx 
+(1 row)
+
+FETCH ABSOLUTE 1 FROM foo24; -- should fail
+ERROR:  cursor can only scan forward
+HINT:  Declare it with SCROLL option to enable backward scan.
+END;
 --
 -- Cursors outside transaction blocks
 --
diff --git a/src/test/regress/expected/tablesample.out b/src/test/regress/expected/tablesample.out
index 078358d226..60bb4e8e3e 100644
--- a/src/test/regress/expected/tablesample.out
+++ b/src/test/regress/expected/tablesample.out
@@ -88,7 +88,7 @@ View definition:

 -- check a sampled query doesn't affect cursor in progress
 BEGIN;
-DECLARE tablesample_cur CURSOR FOR
+DECLARE tablesample_cur SCROLL CURSOR FOR
   SELECT id FROM test_tablesample TABLESAMPLE SYSTEM (50) REPEATABLE (0);
 FETCH FIRST FROM tablesample_cur;
  id
diff --git a/src/test/regress/sql/portals.sql b/src/test/regress/sql/portals.sql
index bf1dff884d..2a098a43b9 100644
--- a/src/test/regress/sql/portals.sql
+++ b/src/test/regress/sql/portals.sql
@@ -190,6 +190,18 @@ FETCH BACKWARD 1 FROM foo24; -- should fail

 END;

+BEGIN;
+
+DECLARE foo24 NO SCROLL CURSOR FOR SELECT * FROM tenk1 ORDER BY unique2;
+
+FETCH 1 FROM foo24;
+
+FETCH ABSOLUTE 2 FROM foo24; -- allowed
+
+FETCH ABSOLUTE 1 FROM foo24; -- should fail
+
+END;
+
 --
 -- Cursors outside transaction blocks
 --
diff --git a/src/test/regress/sql/tablesample.sql b/src/test/regress/sql/tablesample.sql
index c39fe4b750..aa17994277 100644
--- a/src/test/regress/sql/tablesample.sql
+++ b/src/test/regress/sql/tablesample.sql
@@ -24,7 +24,7 @@ CREATE VIEW test_tablesample_v2 AS

 -- check a sampled query doesn't affect cursor in progress
 BEGIN;
-DECLARE tablesample_cur CURSOR FOR
+DECLARE tablesample_cur SCROLL CURSOR FOR
   SELECT id FROM test_tablesample TABLESAMPLE SYSTEM (50) REPEATABLE (0);

 FETCH FIRST FROM tablesample_cur;
diff --git a/src/backend/commands/portalcmds.c b/src/backend/commands/portalcmds.c
index b8590d422b..3224261419 100644
--- a/src/backend/commands/portalcmds.c
+++ b/src/backend/commands/portalcmds.c
@@ -375,6 +375,15 @@ PersistHoldablePortal(Portal portal)
          * can be processed.  Otherwise, store only the not-yet-fetched rows.
          * (The latter is not only more efficient, but avoids semantic
          * problems if the query's output isn't stable.)
+         *
+         * In the no-scroll case, tuple indexes in the tuplestore will not
+         * match the cursor's nominal position (portalPos).  Currently this
+         * causes no difficulty because we only navigate in the tuplestore by
+         * relative position, except for the tuplestore_skiptuples call below
+         * and the tuplestore_rescan call in DoPortalRewind, both of which are
+         * disabled for no-scroll cursors.  But someday we might need to track
+         * the offset between the holdStore and the cursor's nominal position
+         * explicitly.
          */
         if (portal->cursorOptions & CURSOR_OPT_SCROLL)
         {
@@ -382,10 +391,6 @@ PersistHoldablePortal(Portal portal)
         }
         else
         {
-            /* Disallow moving backwards from here */
-            portal->atStart = true;
-            portal->portalPos = 0;
-
             /*
              * If we already reached end-of-query, set the direction to
              * NoMovement to avoid trying to fetch any tuples.  (This check
@@ -443,10 +448,17 @@ PersistHoldablePortal(Portal portal)
         {
             tuplestore_rescan(portal->holdStore);

-            if (!tuplestore_skiptuples(portal->holdStore,
-                                       portal->portalPos,
-                                       true))
-                elog(ERROR, "unexpected end of tuple stream");
+            /*
+             * In the no-scroll case, the start of the tuplestore is exactly
+             * where we want to be, so no repositioning is wanted.
+             */
+            if (portal->cursorOptions & CURSOR_OPT_SCROLL)
+            {
+                if (!tuplestore_skiptuples(portal->holdStore,
+                                           portal->portalPos,
+                                           true))
+                    elog(ERROR, "unexpected end of tuple stream");
+            }
         }
     }
     PG_CATCH();
diff --git a/src/test/regress/expected/portals.out b/src/test/regress/expected/portals.out
index d98c58d303..9da74876e1 100644
--- a/src/test/regress/expected/portals.out
+++ b/src/test/regress/expected/portals.out
@@ -781,6 +781,43 @@ SELECT name, statement, is_holdable, is_binary, is_scrollable FROM pg_cursors;
 (1 row)

 CLOSE foo25;
+BEGIN;
+DECLARE foo25ns NO SCROLL CURSOR WITH HOLD FOR SELECT * FROM tenk2;
+FETCH FROM foo25ns;
+ unique1 | unique2 | two | four | ten | twenty | hundred | thousand | twothousand | fivethous | tenthous | odd | even
|stringu1 | stringu2 | string4  

+---------+---------+-----+------+-----+--------+---------+----------+-------------+-----------+----------+-----+------+----------+----------+---------
+    8800 |       0 |   0 |    0 |   0 |      0 |       0 |      800 |         800 |      3800 |     8800 |   0 |    1
|MAAAAA   | AAAAAA   | AAAAxx 
+(1 row)
+
+FETCH FROM foo25ns;
+ unique1 | unique2 | two | four | ten | twenty | hundred | thousand | twothousand | fivethous | tenthous | odd | even
|stringu1 | stringu2 | string4  

+---------+---------+-----+------+-----+--------+---------+----------+-------------+-----------+----------+-----+------+----------+----------+---------
+    1891 |       1 |   1 |    3 |   1 |     11 |      91 |      891 |        1891 |      1891 |     1891 | 182 |  183
|TUAAAA   | BAAAAA   | HHHHxx 
+(1 row)
+
+COMMIT;
+FETCH FROM foo25ns;
+ unique1 | unique2 | two | four | ten | twenty | hundred | thousand | twothousand | fivethous | tenthous | odd | even
|stringu1 | stringu2 | string4  

+---------+---------+-----+------+-----+--------+---------+----------+-------------+-----------+----------+-----+------+----------+----------+---------
+    3420 |       2 |   0 |    0 |   0 |      0 |      20 |      420 |        1420 |      3420 |     3420 |  40 |   41
|OBAAAA   | CAAAAA   | OOOOxx 
+(1 row)
+
+FETCH ABSOLUTE 4 FROM foo25ns;
+ unique1 | unique2 | two | four | ten | twenty | hundred | thousand | twothousand | fivethous | tenthous | odd | even
|stringu1 | stringu2 | string4  

+---------+---------+-----+------+-----+--------+---------+----------+-------------+-----------+----------+-----+------+----------+----------+---------
+    9850 |       3 |   0 |    2 |   0 |     10 |      50 |      850 |        1850 |      4850 |     9850 | 100 |  101
|WOAAAA   | DAAAAA   | VVVVxx 
+(1 row)
+
+FETCH ABSOLUTE 4 FROM foo25ns; -- fail
+ERROR:  cursor can only scan forward
+HINT:  Declare it with SCROLL option to enable backward scan.
+SELECT name, statement, is_holdable, is_binary, is_scrollable FROM pg_cursors;
+  name   |                              statement                              | is_holdable | is_binary |
is_scrollable 

+---------+---------------------------------------------------------------------+-------------+-----------+---------------
+ foo25ns | DECLARE foo25ns NO SCROLL CURSOR WITH HOLD FOR SELECT * FROM tenk2; | t           | f         | f
+(1 row)
+
+CLOSE foo25ns;
 --
 -- ROLLBACK should close holdable cursors
 --
diff --git a/src/test/regress/sql/portals.sql b/src/test/regress/sql/portals.sql
index 2a098a43b9..eadf6ed942 100644
--- a/src/test/regress/sql/portals.sql
+++ b/src/test/regress/sql/portals.sql
@@ -229,6 +229,26 @@ SELECT name, statement, is_holdable, is_binary, is_scrollable FROM pg_cursors;

 CLOSE foo25;

+BEGIN;
+
+DECLARE foo25ns NO SCROLL CURSOR WITH HOLD FOR SELECT * FROM tenk2;
+
+FETCH FROM foo25ns;
+
+FETCH FROM foo25ns;
+
+COMMIT;
+
+FETCH FROM foo25ns;
+
+FETCH ABSOLUTE 4 FROM foo25ns;
+
+FETCH ABSOLUTE 4 FROM foo25ns; -- fail
+
+SELECT name, statement, is_holdable, is_binary, is_scrollable FROM pg_cursors;
+
+CLOSE foo25ns;
+
 --
 -- ROLLBACK should close holdable cursors
 --