Re: We don't enforce NO SCROLL cursor restrictions

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