From d844cd75a6764a1dd2d8b3410144df9ebf8a0f04 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Fri, 10 Sep 2021 13:18:32 -0400 Subject: [PATCH] Fix some anomalies with NO SCROLL cursors. We have long forbidden fetching backwards from a NO SCROLL cursor, but the prohibition didn't extend to cases in which we rewind the query altogether and then re-fetch forwards. I think the reason is that this logic was mainly meant to protect plan nodes that can't be run in the reverse direction. However, re-reading the query output is problematic if the query is volatile (which includes SELECT FOR UPDATE, not just queries with volatile functions): the re-read can produce different results, which confuses the cursor navigation logic completely. Another reason for disliking this approach is that some code paths will either fetch backwards or rewind-and-fetch-forwards depending on the distance to the target row; so that seemingly identical use-cases may or may not draw the "cursor can only scan forward" error. Hence, let's clean things up by disallowing rewind as well as fetch-backwards in a NO SCROLL cursor. Ordinarily we'd only make such a definitional change in HEAD, but there is a third reason to consider this change now. Commit ba2c6d6ce created some new user-visible anomalies for non-scrollable cursors WITH HOLD, in that navigation in the cursor result got confused if the cursor had been partially read before committing. The only good way to resolve those anomalies is to forbid rewinding such a cursor, which allows removal of the incorrect cursor state manipulations that ba2c6d6ce added to PersistHoldablePortal. To minimize the behavioral change in the back branches (including v14), refuse to rewind a NO SCROLL cursor only when it has a holdStore, ie has been held over from a previous transaction due to WITH HOLD. This should avoid breaking most applications that have been sloppy about whether to declare cursors as scrollable. We'll enforce the prohibition across-the-board beginning in v15. Back-patch to v11, as ba2c6d6ce was. Discussion: https://postgr.es/m/3712911.1631207435@sss.pgh.pa.us --- src/backend/commands/portalcmds.c | 28 ++++++++++++++------ src/backend/tcop/pquery.c | 33 +++++++++++++++++++++--- src/test/regress/expected/portals.out | 37 +++++++++++++++++++++++++++ src/test/regress/sql/portals.sql | 20 +++++++++++++++ 4 files changed, 107 insertions(+), 11 deletions(-) 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/backend/tcop/pquery.c b/src/backend/tcop/pquery.c index a3c27d9d74..57ffd9bc4c 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,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 42dc637fd4..bc52317e3c 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 bf1dff884d..5d1ef52aa5 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 --