ScanCommandId should become part of snapshot

I've been looking at the uses of scanCommandId with some suspicion.
We recently fixed a problem wherein cursors were seeing the effects
of commands started after the cursor was opened (in the same
transaction), and I think there may be more such problems. I don't
like the way that SQL functions and cursors mess around with a global
variable that affects many other places.

I am thinking that ScanCommandId ought to be part of the SnapshotData
struct, and not a global variable at all. In this vision, when
ExecutorStart() saves the current QuerySnapshot into estate->es_snapshot
to define the MVCC worldview for the current query, it should also save
CurrentCommandId into that same datastructure. This action would thus
freeze visibility of effects of both external transactions and commands
of the current transaction for the execution of this query.

Having done this I believe we could remove the scanCommandId global
variable entirely, as well as the save/restore logic that exists for
it in commands/portalcmds.c, executor/functions.c, executor/spi.c.
The places where scanCommandId is restored to a prior value would
not be needed, because the logic they are trying to protect would now
be looking at a previously saved snapshot instead.

There are three tqual.c routines that make use of scanCommandId:
HeapTupleSatisfiesNow, HeapTupleSatisfiesUpdate, and
HeapTupleSatisfiesSnapshot. With scanCommandId in SnapshotData,
obviously HeapTupleSatisfiesSnapshot would have easy access to the
command ID it should check against. In the case of
HeapTupleSatisfiesNow, I believe CurrentCommandId should be used in all
cases. It doesn't make any sense to me that HeapTupleSatisfiesNow will
recognize just-committed tuples from other transactions and not tuples
from past commands of the current transaction. That is: if there is no
snapshotting effect for other transactions then there should be none for
my own commands either. Likewise for HeapTupleSatisfiesUpdate: the
reference ought always to be CurrentCommandId, never anything older.

Comments? Have I missed something?

regards, tom lane