Re: Updatable cursors thoughts

I've been looking at the updatable-cursors patch
http://archives.postgresql.org/pgsql-patches/2007-05/msg00264.php
which attempts to implement the SQL-spec UPDATE/DELETE WHERE CURRENT OF
syntax. It's pretty much of a mess, but there are some salvageable
ideas. There are two big things I don't like about it: one is the
parsetree representation (fails to reverse-list nicely) and the other
is its assumption that updatable cursors must have FOR UPDATE specified.
That is not required by the SQL spec AFAICS, and since our version of
FOR UPDATE is not really compatible with the spec (they say it lists
columns, we say it lists tables), I don't want to require its use in a
patch that is supposedly moving towards spec compliance. Another
objection is that the patch seriously reduces the previously available
functionality of FOR UPDATE by requiring that a cursor mentioning FOR
UPDATE be simply updatable (ie, no joins or grouping). That would cause
compatibility problems for existing apps.

What I think we could do instead is not change any existing behavior of
cursor declarations, but when WHERE CURRENT OF is used, dig through the
execution node tree of the cursor to find the scan node for the target
table. The "dig" would refuse to descend below join or grouping nodes,
and thus implement the restriction that the cursor be simply updatable.
This also means that the digging would be cheap enough that it wouldn't
be a performance bottleneck. This would be enough to implement SQL92's
notion of updatability. SQL2003 has a more complex notion of updatability
that can allow updating of join members in some cases. I'm not sure
whether execution-tree examination could be extended to handle that, but
I suspect that the practical use-cases for WHERE CURRENT OF don't need it
anyway. In any case the currently proposed patch hasn't got any clear
path to supporting that either.

As far as parsetree representation goes, I'm thinking of having the
grammar generate a "CurrentOfExpr" node type that just carries the target
cursor name, and let that propagate as-is through parse analysis. In the
planner or maybe rewriter, expand it to

target_table.ctid = pg_current_of_cursor("cursor name",
target_table.tableoid)

where pg_current_of_cursor is a function (name,oid) returning TID. It
would look up the cursor's portal, dig down to find the scan node, and
return the CTID of the tuple currently stored in the scan node's scan
tuple slot. Passing the tableoid is necessary to handle inheritance cases
--- the cursor plan could involve an Append and we have to be able to tell
which Append member to look at. Also, the function has to return NULL
(preventing a match) when invoked for some other member of the inheritance
tree than the one currently being scanned.

The reason for the CurrentOfExpr node representation before planning is
to have something that ruleutils.c can reverse-list into WHERE CURRENT OF,
instead of some nonstandard comparison on ctid.

Comments?

regards, tom lane

On Sat, 2007-06-09 at 16:25 -0400, Tom Lane wrote:

> What I think we could do instead is not change any existing behavior of
> cursor declarations, but when WHERE CURRENT OF is used, dig through the
> execution node tree of the cursor to find the scan node for the target
> table.

Sounds good. The cost is paid by the WHERE CURRENT OF query, not as an
overhead on all cursors. Sounds like it will still be very fast too.

Presumably still disallowing the WITH HOLD case.

--
Simon Riggs
EnterpriseDB http://www.enterprisedb.com

"Simon Riggs" <simon(at)2ndquadrant(dot)com> writes:
> Sounds good. The cost is paid by the WHERE CURRENT OF query, not as an
> overhead on all cursors. Sounds like it will still be very fast too.

Yeah, there's zero added cost in the existing code paths, and the lookup
isn't really that expensive.

> Presumably still disallowing the WITH HOLD case.

Right --- once you've committed, you can't be sure the rows are still
there to look at.

I just finished revising the patch and am about to commit it. In the
event, my idea of a separate lookup function didn't work so well --- it
needed to be STABLE to be used in a tidscan qual, but it also needed to
be VOLATILE to prevent the planner from speculatively executing it
(resulting in premature failure if the cursor wasn't there at plan
time). So I ended up letting the special CurrentOfExpr node type
persist all the way through to execution. This required very minor
hackery in tidpath.c and nodeTidscan.c to accept such a node as a TID
qual; it would've taken something uglier to make the function
representation work.

One area that could still do with more work is getting the facility to
play nicely with views. However, it's probably premature to think about
that before we have updatable views that work well...

regards, tom lane