We don't enforce NO SCROLL cursor restrictions

On 9/9/21 7:10 PM, Tom Lane wrote:

We don't actually prevent you from scrolling a NO SCROLL cursor:

There are probably specific cases where you do get an error,
but we don't have a blanket you-can't-do-that check. Should we?

I would say yes. NO SCROLL means no scrolling; or at least should.

On the other hand, if there is no optimization or other meaningful
difference between SCROLL and NO SCROLL, then we can just document it as
a no-op that is only provided for standard syntax compliance.
--
Vik Fearing

Vik Fearing <vik@postgresfriends.org> writes:

On 9/9/21 7:10 PM, Tom Lane wrote:

There are probably specific cases where you do get an error,
but we don't have a blanket you-can't-do-that check. Should we?

I would say yes. NO SCROLL means no scrolling; or at least should.
On the other hand, if there is no optimization or other meaningful
difference between SCROLL and NO SCROLL, then we can just document it as
a no-op that is only provided for standard syntax compliance.

There are definitely optimizations that happen or don't happen
depending on the SCROLL option. I think ba2c6d6ce may be the
first patch that introduces any user-visible semantic difference,
but I'm not completely sure about that.

[ pokes at it some more ... ] Hm, we let you do this:

regression=# begin;
BEGIN
regression=*# declare c cursor for select * from int8_tbl for update;
DECLARE CURSOR
regression=*# fetch all from c;
q1 | q2
------------------+-------------------
123 | 456
123 | 4567890123456789
4567890123456789 | 123
4567890123456789 | 4567890123456789
4567890123456789 | -4567890123456789
(5 rows)

regression=*# fetch absolute 2 from c;
q1 | q2
-----+------------------
123 | 4567890123456789
(1 row)

which definitely flies in the face of the fact that we disallow
combining SCROLL and FOR UPDATE:

regression=*# declare c scroll cursor for select * from int8_tbl for update;
ERROR: DECLARE SCROLL CURSOR ... FOR UPDATE is not supported
DETAIL: Scrollable cursors must be READ ONLY.

I don't recall the exact reason for that prohibition, but I wonder
if there aren't user-visible anomalies reachable from the fact that
you can bypass it.

regards, tom lane

I wrote:

[ pokes at it some more ... ] Hm, we let you do this:
...
which definitely flies in the face of the fact that we disallow
combining SCROLL and FOR UPDATE:
regression=*# declare c scroll cursor for select * from int8_tbl for update;
ERROR: DECLARE SCROLL CURSOR ... FOR UPDATE is not supported
DETAIL: Scrollable cursors must be READ ONLY.

I don't recall the exact reason for that prohibition, but I wonder
if there aren't user-visible anomalies reachable from the fact that
you can bypass it.

I dug in the archives. The above-quoted error message was added by
me in 048efc25e, responding to Heikki's complaint here:

/messages/by-id/471F69FE.5000500@enterprisedb.com

What I now see is that I put that check at the wrong level. It
successfully blocks off the case Heikki complained of:

DROP TABLE IF EXISTS foo;
CREATE TABLE foo (id integer);
INSERT INTO foo SELECT a from generate_series(1,10) a;
BEGIN;
DECLARE c CURSOR FOR SELECT id FROM foo FOR UPDATE;
FETCH 2 FROM c;
UPDATE foo set ID=20 WHERE CURRENT OF c;
FETCH RELATIVE 0 FROM c;
COMMIT;

The FETCH RELATIVE 0 fails with

ERROR: cursor can only scan forward
HINT: Declare it with SCROLL option to enable backward scan.

However, if you replace that with the should-be-equivalent

FETCH ABSOLUTE 2 FROM c;

then what you get is not an error but

id
----
3
(1 row)

which is for certain anomalous, because that is not the row you
saw as being row 2 in the initial FETCH.

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. And, since the query
is using FOR UPDATE, this table scan sees the row with ID=2 as already
dead. (Its replacement with ID=20 has been installed at the end of
the table, so while it would be visible to the cursor, it's not at
the same position as before.)

So basically, we *do* have this check and have done since 2007,
but it's not water-tight for all variants of FETCH. I think
tightening it up in HEAD and v14 is a no-brainer, but I'm a bit
more hesitant about whether to back-patch into stable branches.

regards, tom lane

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