Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Can I get some comments on this?
It's far from ready for prime time :-(
The main problem with it is it's doing the wrong things in the wrong
places. The key part of the patch is a hack in parse_expr.c to
transform "WHERE CURRENT OF cursor" into "WHERE ctid = <tid constant>",
where the TID constant has been obtained by reaching into the cursor and
grabbing the current position. Parse analysis is entirely the wrong
time to be doing that --- in any context where the statement will not be
executed immediately, there is no good reason to expect that the cursor
even exists, let alone is positioned at the row it will be positioned at
when you finally execute the query. This approach cannot support WHERE
CURRENT OF in plpgsql functions, rules, or prepared statements.
A lesser but still fatal objection is that the rule reverse-lister would
show the query as "WHERE ctid = <tid constant>" rather than "WHERE
CURRENT OF".
AFAICS, WHERE CURRENT OF has to be a construct that propagates in just
that form all the way into the executor (with a hack in the planner to
ensure that a TidScan plan is chosen) and then the reaching into the
cursor has to happen at plan startup in nodeTidscan.c.
The business with a substitute snapshot seems pretty dubious too. It
looks like a cursor that started with a normal snapshot --- some
transactions visible, some not --- will suddenly flip into SnapshotNow
except for the specific row(s) updated by the current transaction.
That's not gonna do. In the first place, only rows updated *through
that cursor* ought to change visibility, according to my reading of the
spec. In the second place, SnapshotNow will allow visibility of
external transactions that should not have been visible according to the
original snapshot.
I don't understand what the diffs in postgres.c are supposed to
accomplish, but I can just about promise that they are misplaced too,
since they won't affect queries executed via functions or SPI.
regards, tom lane