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

On Fri, 21 Sep 2001, Tom Lane wrote:
>
> 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.
Good point. I'll work on it.

> 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.)
Yeah, I couldn't figure out which option is worse, which is why I didn't
implement it. I think that rewinding the cursor on each query is better,
but I wanted to get comments first.

> 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.
OK, giving an error makes sense.

> 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.)
OK.

> 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.

If you want to consider this argument: It won't break anything that's not
using the feature. It is needed (its not a 'fringe change' to benefit few)
(well, at least I think so :).

It also is a base for my code to do 'select * from func(args)', which is
definitely needed and base of many flames against postgres not having
'real' stored procedures (ones that return sets). I was hoping to get the
rest of it in 7.2 so these flames can be put to rest.

Changes to core code are obvious, and all documentation can be taken care
of during beta.

But I understand your apprehension...

> 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 :-(
Can't blame you, completely understandable with GB situation...