Horton

Thread

  1. [PATCH] [LARGE] select * from cursor foo

    Alex Pilosov <alex@pilosoft.com> — 2001-08-29T04:23:34Z

    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

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

    Bruce Momjian <pgman@candle.pha.pa.us> — 2001-09-04T05:00:49Z

    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, Pennsylvania 19026

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2001-09-17T21:15:22Z

    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

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

    Alex Pilosov <alex@pilosoft.com> — 2001-09-21T02:08:51Z

    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      |

  5. CVS/CVSup problems (was Re: [PATCH] [LARGE] )

    Alex Pilosov <alex@pilosoft.com> — 2001-09-21T02:12:23Z

    On Thu, 20 Sep 2001, Alex Pilosov wrote:

> 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?
Augh. A minute later, I find the announcement, but I still have problem,
after logging in, I have: 

tick-bash# cvs -d :pserver:anoncvs@anoncvs.postgresql.org:/projects/cvsroot co pgsql
cvs server: Updating pgsql
cvs server: failed to create lock directory for `/projects/cvsroot/pgsql'
(/projects/cvsroot/pgsql/#cvs.lock): Permission denied
cvs server: failed to obtain dir lock in repository
`/projects/cvsroot/pgsql'
cvs [server aborted]: read lock failed - giving up

A minute later:
cannot create_adm_p /tmp/cvs-serv8577/pgsql

...:(

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

    Thomas Lockhart <lockhart@fourpalms.org> — 2001-09-21T05:04:19Z

    > 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

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

    Alex Pilosov <alex@pilosoft.com> — 2001-09-21T19:11:10Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2001-09-21T22:26:22Z

    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

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

    Alex Pilosov <alex@pilosoft.com> — 2001-09-22T02:12:13Z

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

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

    Bruce Momjian <pgman@candle.pha.pa.us> — 2002-02-22T13:09:26Z

    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, Pennsylvania 19026

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

    Bruce Momjian <pgman@candle.pha.pa.us> — 2002-02-25T03:54:28Z

    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, Pennsylvania 19026