Thread: [PATCH] [LARGE] select * from cursor foo
Attached patch does the above. Notes: 1. Incompatible changes: CURSOR is now a keyword and may not be used as an identifier (tablename, etc). Otherwise, we get shift-reduce conflicts in grammar. 2. Major changes: a) RangeTblEntry (RTE for short) instead of having two possibilities, subquery and non-subquery, now has a rtetype field which can be of 3 possible states: RTE_RELATION, RTE_SUBSELECT, RTE_PORTAL). The type-specific structures are unionized, so where you used to have rte->relid, now you must do rte->u.rel.relid. Proper way to check what is the RTE type is now checking for rte->rtetype instead of checking whether rte->subquery is null. b) Similarly, RelOptInfo now has a RelOptInfoType which is an enum with 4 states, REL_PLAIN,REL_SUBQUERY,REL_JOIN,REL_PORTAL. I did not do the unionization of type-specific structures. Maybe I should've if I'm going to get in a big change anyway. c) There's a function PortalRun which fetches N records from portal and sets atEnd/atStart values properly. It replaces code duplicated in 2 places. How to test: declare foo cursor for select * from pg_class; select * from cursor foo; Documentation updates will be forthcoming ASAP, I just wanted to get this patch in queue before the freeze. Or at least making sure someone could look through this patch before freeze. :) Next patch will be one to support "SELECT * FROM func(arg1,arg2)" which would work by creating first a special kind of portal for selection from a function and then setting query source to be that portal. -alex
Your patch has been added to the PostgreSQL unapplied patches list at: http://candle.pha.pa.us/cgi-bin/pgpatches I will try to apply it within the next 48 hours. > Attached patch does the above. > > Notes: > 1. Incompatible changes: CURSOR is now a keyword and may not be used as an > identifier (tablename, etc). Otherwise, we get shift-reduce conflicts in > grammar. > > 2. Major changes: > > a) RangeTblEntry (RTE for short) instead of having two possibilities, > subquery and non-subquery, now has a rtetype field which can be of 3 > possible states: RTE_RELATION, RTE_SUBSELECT, RTE_PORTAL). The > type-specific structures are unionized, so where you used to have > rte->relid, now you must do rte->u.rel.relid. > > Proper way to check what is the RTE type is now checking for rte->rtetype > instead of checking whether rte->subquery is null. > > b) Similarly, RelOptInfo now has a RelOptInfoType which is an enum with 4 > states, REL_PLAIN,REL_SUBQUERY,REL_JOIN,REL_PORTAL. I did not do the > unionization of type-specific structures. Maybe I should've if I'm going > to get in a big change anyway. > > c) There's a function PortalRun which fetches N records from portal and > sets atEnd/atStart values properly. It replaces code duplicated in 2 > places. > > > How to test: > > declare foo cursor for select * from pg_class; > > select * from cursor foo; > > Documentation updates will be forthcoming ASAP, I just wanted to get this > patch in queue before the freeze. Or at least making sure someone could > look through this patch before freeze. :) > > Next patch will be one to support "SELECT * FROM func(arg1,arg2)" which > would work by creating first a special kind of portal for selection from a > function and then setting query source to be that portal. > > -alex Content-Description: [ Attachment, skipping... ] > > ---------------------------(end of broadcast)--------------------------- > TIP 5: Have you checked our extensive FAQ? > > http://www.postgresql.org/users-lounge/docs/faq.html -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026
Alex Pilosov <alex@pilosoft.com> writes:
> Attached patch does the above.
Alex, could we have this resubmitted in "diff -c" format? Plain diff
format is way too risky to apply.
regards, tom lane
On Mon, 17 Sep 2001, Tom Lane wrote: > Alex Pilosov <alex@pilosoft.com> writes: > > Attached patch does the above. > > Alex, could we have this resubmitted in "diff -c" format? Plain diff > format is way too risky to apply. Tom, postgresql.org cvsup repository is broken (and according to my records, been so for last 4 days at least). Unfortunately, I can't get my changes in correct format unless that gets fixed...So I guess that'll go in 7.3 :( (error I'm getting is this: Server message: Collection "pgsql" release "cvs" is not available here which leads me to assume a misconfiguration somewhere). That is, unless you make an exception for a few days and cvsup gets fixed in meantime :P) CVS repository also seems broken right now, I'm unable to log in (cvs login: authorization failed: server cvs.postgresql.org rejected access to /home/projects/pgsql/cvsroot for user anoncvs) in both cvs.postgresql.org and anoncvs.postgresql.org with all possible anoncvs passwords (empty, anoncvs, postgresql). Or had the anoncvs password been changed, or I missed an announcement? -- Alex Pilosov | http://www.acedsl.com/home.html CTO - Acecape, Inc. | AceDSL:The best ADSL in the world 325 W 38 St. Suite 1005 | (Stealth Marketing Works! :) New York, NY 10018 |
> postgresql.org cvsup repository is broken (and according to my records,
> been so for last 4 days at least). Unfortunately, I can't get my changes
> in correct format unless that gets fixed...So I guess that'll go in 7.3 :(
The beta release schedule is on hold until we can get access. I've
wasted a few days tracking down a problem which turns out to have been
fixed in cvs, but since I don't have CVSup access (just like you) I
can't get the updates. And I can't finish testing and can't finish
updating the regression suite, etc etc.
- Thomas
Alex Pilosov <alex@pilosoft.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
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...
On Mon, 17 Sep 2001, Tom Lane wrote: > Alex Pilosov <alex@pilosoft.com> writes: > > Attached patch does the above. > > 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 Thanks -alex
Alex, can I have an updated version of this patch for application to 7.3? --------------------------------------------------------------------------- Alex Pilosov wrote: > 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... > > > > > ---------------------------(end of broadcast)--------------------------- > TIP 2: you can get off all lists at once with the unregister command > (send "unregister YourEmailAddressHere" to majordomo@postgresql.org) > -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026
Sorry, this needs more work. Please continue discussion on hackers. --------------------------------------------------------------------------- Alex Pilosov wrote: > On Mon, 17 Sep 2001, Tom Lane wrote: > > > Alex Pilosov <alex@pilosoft.com> writes: > > > Attached patch does the above. > > > > 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 > > > Thanks > -alex Content-Description: [ Attachment, skipping... ] > > ---------------------------(end of broadcast)--------------------------- > TIP 5: Have you checked our extensive FAQ? > > http://www.postgresql.org/users-lounge/docs/faq.html -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026