Re: [PATCH] [LARGE] select * from cursor foo

Alex Pilosov <alex(at)pilosoft(dot)com> writes:
>> Alex, could we have this resubmitted in "diff -c" format? Plain diff
>> format is way too risky to apply.

> Resubmitted. In case list server chokes on the attachment's size, it is
> also available on www.formenos.org/pg/cursor.fix5.diff

I've looked this over, and I think it's not mature enough to apply at
this late stage of the 7.2 cycle; we'd better hold it over for more work
during 7.3. Major problems:

1. Insufficient defense against queries that outlive the cursors they
select from. For example, I could create a view that selects from a
cursor. Yes, you check to see if the cursor name still exists ... but
what if that name now refers to a new cursor that delivers a completely
different set of columns? Instant coredump.

2. I don't understand the semantics of queries that read cursors
that've already had some rows fetched from them. Should we reset the
cursor to the start, so that all its data is implicitly available?
That seems strange, but if we don't do it, I think the behavior will be
quite unpredictable, since some join types are going to result in
resetting and re-reading the cursor multiple times anyway. (You've
punted on this issue by not implementing ExecPortalReScan, but that's
not acceptable for a production feature.)

3. What does it mean to SELECT FOR UPDATE from a cursor? I don't think
ignoring the FOR UPDATE spec is acceptable; maybe we just have to raise
an error.

4. Complete lack of documentation, including lack of attention to
updating the code's internal documentation. (For instance, you seem
to have changed some of the conventions described in nodes/relation.h,
but you didn't fix those comments.)

The work you have done so far on changing RTE etc looks good ... but
I don't think the patch is ready to go. Nor am I comfortable with
applying it now on the assumption that the problems can be fixed during
beta.

I realize you originally sent this in a month ago, and perhaps you would
have had time to respond to these concerns if people had reviewed the
patch promptly. For myself, I can only apologize for not getting to it
sooner. I've had a few distractions over the past month :-(

regards, tom lane