diff --git a/src/backend/commands/portalcmds.c b/src/backend/commands/portalcmds.c index 377cf9ae9d..4f3e0be5de 100644 --- a/src/backend/commands/portalcmds.c +++ b/src/backend/commands/portalcmds.c @@ -381,6 +381,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) { @@ -388,10 +397,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 @@ -447,10 +452,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/backend/tcop/pquery.c b/src/backend/tcop/pquery.c index 697430e927..5781dcde7c 100644 --- a/src/backend/tcop/pquery.c +++ b/src/backend/tcop/pquery.c @@ -1469,9 +1469,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) */ @@ -1488,6 +1487,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: @@ -1665,6 +1673,25 @@ DoPortalRewind(Portal portal) { QueryDesc *queryDesc; + /* + * No work is needed if we've not advanced nor attempted to advance the + * cursor (and we don't want to throw a NO SCROLL error in this case). + */ + if (portal->atStart && !portal->atEnd) + return; + + /* + * Otherwise, cursor should allow scrolling. However, we're only going to + * enforce that policy fully beginning in v15. In older branches, insist + * on this only if the portal has a holdStore. That prevents users from + * seeing that the holdStore may not have all the rows of the query. + */ + if ((portal->cursorOptions & CURSOR_OPT_NO_SCROLL) && portal->holdStore) + 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 dc0d2ef7dd..7f3426c0e6 100644 --- a/src/test/regress/expected/portals.out +++ b/src/test/regress/expected/portals.out @@ -763,6 +763,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 52560ac027..e4607fc302 100644 --- a/src/test/regress/sql/portals.sql +++ b/src/test/regress/sql/portals.sql @@ -217,6 +217,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 --