Horton
Menu

Thread

  1. [PATCH] Provide rowcount for utility SELECTs

    Boszormenyi Zoltan <zb@cybertec.at> — 2009-12-28T09:57:58Z

    Hi,

attached is a small patch that makes it possible for clients
to receive row count for SELECT ... INTO ... and CREATE TABLE ... AS ...

Comments?

Best regards,
Zoltán Böszörményi

-- 
Bible has answers for everything. Proof:
"But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
than these cometh of evil." (Matthew 5:37) - basics of digital technology.
"May your kingdom come" - superficial description of plate tectonics

----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/

  2. Re: [PATCH] Provide rowcount for utility SELECTs

    Pavel Stehule <pavel.stehule@gmail.com> — 2009-12-28T10:08:13Z

    2009/12/28 Boszormenyi Zoltan <zb@cybertec.at>:
> Hi,
>
> attached is a small patch that makes it possible for clients
> to receive row count for SELECT ... INTO ... and CREATE TABLE ... AS ...
>
> Comments?
>

good idea

+1

Pavel

> Best regards,
> Zoltán Böszörményi
>
> --
> Bible has answers for everything. Proof:
> "But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
> than these cometh of evil." (Matthew 5:37) - basics of digital technology.
> "May your kingdom come" - superficial description of plate tectonics
>
> ----------------------------------
> Zoltán Böszörményi
> Cybertec Schönig & Schönig GmbH
> http://www.postgresql.at/
>
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>

  3. Re: [PATCH] Provide rowcount for utility SELECTs

    Hans-Juergen Schoenig <hs@cybertec.at> — 2009-12-28T10:17:22Z

    hello ...

just as a background info: this will have some positive side effects on 
embedded C programs which should be portable.
informix, for instance, will also return a row count on those commands.

    regards,

       hans



Pavel Stehule wrote:
> 2009/12/28 Boszormenyi Zoltan <zb@cybertec.at>:
>   
>> Hi,
>>
>> attached is a small patch that makes it possible for clients
>> to receive row count for SELECT ... INTO ... and CREATE TABLE ... AS ...
>>
>> Comments?
>>
>>     
>
> good idea
>
> +1
>
> Pavel
>
>   
>> Best regards,
>> Zoltán Böszörményi
>>
>> --
>> Bible has answers for everything. Proof:
>> "But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
>> than these cometh of evil." (Matthew 5:37) - basics of digital technology.
>> "May your kingdom come" - superficial description of plate tectonics
>>
>> ----------------------------------
>> Zoltán Böszörményi
>> Cybertec Schönig & Schönig GmbH
>> http://www.postgresql.at/
>>
>>
>>
>> --
>> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
>> To make changes to your subscription:
>> http://www.postgresql.org/mailpref/pgsql-hackers
>>
>>
>>     
>
>   


-- 
Cybertec Schoenig & Schoenig GmbH
Reyergasse 9 / 2
A-2700 Wiener Neustadt
Web: www.postgresql-support.de

  4. Re: [PATCH] Provide rowcount for utility SELECTs

    Boszormenyi Zoltan <zb@cybertec.at> — 2009-12-28T10:20:57Z

    Hans-Juergen Schoenig írta:
> hello ...
>
> just as a background info: this will have some positive side effects
> on embedded C programs which should be portable.

Not just embedded C programs, every driver that's
based on libpq and used PQcmdTuples() will
automatically see the benefit.

> informix, for instance, will also return a row count on those commands.
>
>    regards,
>
>       hans
>
>
>
> Pavel Stehule wrote:
>> 2009/12/28 Boszormenyi Zoltan <zb@cybertec.at>:
>>  
>>> Hi,
>>>
>>> attached is a small patch that makes it possible for clients
>>> to receive row count for SELECT ... INTO ... and CREATE TABLE ... AS
>>> ...
>>>
>>> Comments?
>>>
>>>     
>>
>> good idea
>>
>> +1
>>
>> Pavel
>>
>>  
>>> Best regards,
>>> Zoltán Böszörményi
>>>
>>> -- 
>>> Bible has answers for everything. Proof:
>>> "But let your communication be, Yea, yea; Nay, nay: for whatsoever
>>> is more
>>> than these cometh of evil." (Matthew 5:37) - basics of digital
>>> technology.
>>> "May your kingdom come" - superficial description of plate tectonics
>>>
>>> ----------------------------------
>>> Zoltán Böszörményi
>>> Cybertec Schönig & Schönig GmbH
>>> http://www.postgresql.at/
>>>
>>>
>>>
>>> -- 
>>> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
>>> To make changes to your subscription:
>>> http://www.postgresql.org/mailpref/pgsql-hackers
>>>
>>>
>>>     
>>
>>   
>
>


-- 
Bible has answers for everything. Proof:
"But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
than these cometh of evil." (Matthew 5:37) - basics of digital technology.
"May your kingdom come" - superficial description of plate tectonics

----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/

  5. Re: [PATCH] Provide rowcount for utility SELECTs

    Tom Lane <tgl@sss.pgh.pa.us> — 2009-12-28T16:08:19Z

    Boszormenyi Zoltan <zb@cybertec.at> writes:
> Hans-Juergen Schoenig rta:
>> just as a background info: this will have some positive side effects
>> on embedded C programs which should be portable.

> Not just embedded C programs, every driver that's
> based on libpq and used PQcmdTuples() will
> automatically see the benefit.

And, by the same token, the scope for possibly breaking clients is nearly
unlimited ...

			regards, tom lane

  6. Re: [PATCH] Provide rowcount for utility SELECTs

    Peter Eisentraut <peter_e@gmx.net> — 2009-12-28T16:14:04Z

    On mån, 2009-12-28 at 11:08 -0500, Tom Lane wrote:
> Boszormenyi Zoltan <zb@cybertec.at> writes:
> > Hans-Juergen Schoenig írta:
> >> just as a background info: this will have some positive side effects
> >> on embedded C programs which should be portable.
> 
> > Not just embedded C programs, every driver that's
> > based on libpq and used PQcmdTuples() will
> > automatically see the benefit.
> 
> And, by the same token, the scope for possibly breaking clients is nearly
> unlimited ...

Why is that?  Are there programs out there that expect PQcmdTuples() to
return something that is *not* the tuple count for these commands and
will violently misbehave otherwise?

  7. Re: [PATCH] Provide rowcount for utility SELECTs

    Tom Lane <tgl@sss.pgh.pa.us> — 2009-12-28T16:20:19Z

    Peter Eisentraut <peter_e@gmx.net> writes:
> On mn, 2009-12-28 at 11:08 -0500, Tom Lane wrote:
>> And, by the same token, the scope for possibly breaking clients is nearly
>> unlimited ...

> Why is that?  Are there programs out there that expect PQcmdTuples() to
> return something that is *not* the tuple count for these commands and
> will violently misbehave otherwise?

It's more the possibility of doing strcmp(tag, "SELECT") on the command
tag that worries me.  Describing the API change here as being limited
to PQcmdTuples misses the point rather completely: this is a protocol
change, and could break both clients and non-libpq driver libraries.

			regards, tom lane

  8. Re: [PATCH] Provide rowcount for utility SELECTs

    Tom Lane <tgl@sss.pgh.pa.us> — 2009-12-28T17:10:29Z

    Boszormenyi Zoltan <zb@cybertec.at> writes:
> attached is a small patch that makes it possible for clients
> to receive row count for SELECT ... INTO ... and CREATE TABLE ... AS ...

> Comments?

This doesn't look tremendously well thought out to me.

1. As given, the patch changes the result not only for SELECT INTO but
for any SELECT executed in PORTAL_MULTI_QUERY context (consider SELECTs
added by rules for example).  It seems like a pretty bad idea for the
result of a statement to depend on context.

2. In the past we have regretted it when we made the same command tag
sometimes have numbers attached and sometimes not (note the hack at
the bottom of PortalRunMulti).  It doesn't seem like terribly good
design to do that here.  On the other hand, always attaching a count
to SELECT tags would greatly increase the risk of breaking clients.


I'm not at all convinced that this is so useful as to justify taking
any compatibility risks for.  People who really need that count can
get it easily enough by breaking the command into a CREATE followed
by INSERT/SELECT.

			regards, tom lane

  9. Re: [PATCH] Provide rowcount for utility SELECTs

    Boszormenyi Zoltan <zb@cybertec.at> — 2009-12-28T18:29:01Z

    Tom Lane írta:
> Peter Eisentraut <peter_e@gmx.net> writes:
>   
>> On mĂĽn, 2009-12-28 at 11:08 -0500, Tom Lane wrote:
>>     
>>> And, by the same token, the scope for possibly breaking clients is nearly
>>> unlimited ...
>>>       
>
>   
>> Why is that?  Are there programs out there that expect PQcmdTuples() to
>> return something that is *not* the tuple count for these commands and
>> will violently misbehave otherwise?
>>     
>
> It's more the possibility of doing strcmp(tag, "SELECT") on the command
>   

Actually it's strncmp(tag, "SELECT ", 7), so when you mix old server
with new clients or new server with old client, it will just work as
before, i.e.
return "".

> tag that worries me.  Describing the API change here as being limited
> to PQcmdTuples misses the point rather completely: this is a protocol
> change, and could break both clients and non-libpq driver libraries.
>
> 			regards, tom lane
>
>   


-- 
Bible has answers for everything. Proof:
"But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
than these cometh of evil." (Matthew 5:37) - basics of digital technology.
"May your kingdom come" - superficial description of plate tectonics

----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/

  10. Re: [PATCH] Provide rowcount for utility SELECTs

    Tom Lane <tgl@sss.pgh.pa.us> — 2009-12-28T20:18:08Z

    Boszormenyi Zoltan <zb@cybertec.at> writes:
> Tom Lane rta:
>> It's more the possibility of doing strcmp(tag, "SELECT") on the command

> Actually it's strncmp(tag, "SELECT ", 7), so when you mix old server
> with new clients or new server with old client, it will just work as
> before, i.e. return "".

Are you deliberately ignoring the point?  We have no way to know whether
there is any client-side code that's doing a simple check for SELECT
command tag, but it's certainly possible.  The fact that it wouldn't be
hard to fix does not mean that it wouldn't be broken.

			regards, tom lane

  11. Re: [PATCH] Provide rowcount for utility SELECTs

    Boszormenyi Zoltan <zb@cybertec.at> — 2009-12-29T07:50:49Z

    Tom Lane írta:
> Boszormenyi Zoltan <zb@cybertec.at> writes:
>   
>> Tom Lane írta:
>>     
>>> It's more the possibility of doing strcmp(tag, "SELECT") on the command
>>>       
>
>   
>> Actually it's strncmp(tag, "SELECT ", 7), so when you mix old server
>> with new clients or new server with old client, it will just work as
>> before, i.e. return "".
>>     
>
> Are you deliberately ignoring the point?

No, I just thought you were commenting on my patch's details...

>   We have no way to know whether
> there is any client-side code that's doing a simple check for SELECT
> command tag, but it's certainly possible.  The fact that it wouldn't be
> hard to fix does not mean that it wouldn't be broken.
>
> 			regards, tom lane
>
>   


-- 
Bible has answers for everything. Proof:
"But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
than these cometh of evil." (Matthew 5:37) - basics of digital technology.
"May your kingdom come" - superficial description of plate tectonics

----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/

  12. Re: [PATCH] Provide rowcount for utility SELECTs

    Boszormenyi Zoltan <zb@cybertec.at> — 2009-12-29T07:58:14Z

    Tom Lane írta:
> Boszormenyi Zoltan <zb@cybertec.at> writes:
>   
>> attached is a small patch that makes it possible for clients
>> to receive row count for SELECT ... INTO ... and CREATE TABLE ... AS ...
>>     
>
>   
>> Comments?
>>     
>
> This doesn't look tremendously well thought out to me.
>
> 1. As given, the patch changes the result not only for SELECT INTO but
> for any SELECT executed in PORTAL_MULTI_QUERY context (consider SELECTs
> added by rules for example).  It seems like a pretty bad idea for the
> result of a statement to depend on context.
>
> 2. In the past we have regretted it when we made the same command tag
> sometimes have numbers attached and sometimes not (note the hack at
> the bottom of PortalRunMulti).  It doesn't seem like terribly good
> design to do that here.  On the other hand, always attaching a count
> to SELECT tags would greatly increase the risk of breaking clients.
>   

Okay, how about introducing a new "SELECTINTO N" command tag, then?
It's also a protocol change, but at least it can fall into the very last
"else"
anywhere, hence have a high chance of being ignored and handled the same
way as other not rowcount-returning tags.

> I'm not at all convinced that this is so useful as to justify taking
> any compatibility risks for.  People who really need that count can
> get it easily enough by breaking the command into a CREATE followed
> by INSERT/SELECT.
>   

Yes, and every WITH RECURSIVE statement can also be broken up
manually as well. It's simply shorter and has a chance of being a little
more resource-effective:
- one parsing/planning phase instead of two on the server side
- one error checking in the app instead of two
- PostgreSQL already has the infrastructure to return the rowcount

Best regards,
Zoltán Böszörményi

-- 
Bible has answers for everything. Proof:
"But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
than these cometh of evil." (Matthew 5:37) - basics of digital technology.
"May your kingdom come" - superficial description of plate tectonics

----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/

  13. Re: [PATCH] Provide rowcount for utility SELECTs

    Robert Haas <robertmhaas@gmail.com> — 2009-12-29T08:06:21Z

    On Mon, Dec 28, 2009 at 12:10 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Boszormenyi Zoltan <zb@cybertec.at> writes:
>> attached is a small patch that makes it possible for clients
>> to receive row count for SELECT ... INTO ... and CREATE TABLE ... AS ...
>
>> Comments?
>
> This doesn't look tremendously well thought out to me.
>
> 1. As given, the patch changes the result not only for SELECT INTO but
> for any SELECT executed in PORTAL_MULTI_QUERY context (consider SELECTs
> added by rules for example).  It seems like a pretty bad idea for the
> result of a statement to depend on context.
>
> 2. In the past we have regretted it when we made the same command tag
> sometimes have numbers attached and sometimes not (note the hack at
> the bottom of PortalRunMulti).  It doesn't seem like terribly good
> design to do that here.  On the other hand, always attaching a count
> to SELECT tags would greatly increase the risk of breaking clients.
>
>
> I'm not at all convinced that this is so useful as to justify taking
> any compatibility risks for.  People who really need that count can
> get it easily enough by breaking the command into a CREATE followed
> by INSERT/SELECT.

I don't know whether or not it's a good idea to do this for anything
in a PORTAL_MULTI_QUERY context, because I haven't really thought
through the issues.  But, it's hard for me to imagine that anyone is
depending on the command tag returned by CTAS or SELECT INTO.

Generally, I think making command tags return more useful information
is a good thing.

...Robert

  14. Re: [PATCH] Provide rowcount for utility SELECTs

    Alvaro Herrera <alvherre@commandprompt.com> — 2009-12-29T13:35:52Z

    Tom Lane escribió:
> Boszormenyi Zoltan <zb@cybertec.at> writes:
> > Tom Lane rta:
> >> It's more the possibility of doing strcmp(tag, "SELECT") on the command
> 
> > Actually it's strncmp(tag, "SELECT ", 7), so when you mix old server
> > with new clients or new server with old client, it will just work as
> > before, i.e. return "".
> 
> Are you deliberately ignoring the point?  We have no way to know whether
> there is any client-side code that's doing a simple check for SELECT
> command tag, but it's certainly possible.  The fact that it wouldn't be
> hard to fix does not mean that it wouldn't be broken.

But it would be broken in very obvious ways, no?  It's not like it would
be silently broken and thus escape testing ...

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

  15. Re: [PATCH] Provide rowcount for utility SELECTs

    Tom Lane <tgl@sss.pgh.pa.us> — 2009-12-29T15:24:38Z

    Alvaro Herrera <alvherre@commandprompt.com> writes:
> But it would be broken in very obvious ways, no?  It's not like it would
> be silently broken and thus escape testing ...

Well, if we wanted to adopt that approach, we should add the count to
*all* SELECT tags not just a small subset.  As the patch stands it
seems entirely possible that a breakage would escape immediate notice.

			regards, tom lane

  16. Re: [PATCH] Provide rowcount for utility SELECTs

    Boszormenyi Zoltan <zb@cybertec.at> — 2010-01-12T14:23:19Z

    Tom Lane írta:
> Alvaro Herrera <alvherre@commandprompt.com> writes:
>   
>> But it would be broken in very obvious ways, no?  It's not like it would
>> be silently broken and thus escape testing ...
>>     
>
> Well, if we wanted to adopt that approach, we should add the count to
> *all* SELECT tags not just a small subset.  As the patch stands it
> seems entirely possible that a breakage would escape immediate notice.
>
> 			regards, tom lane
>   

Can you give me an example that would return
plain "SELECT" after my new patch? I added
one more change to the patch, is it enough to return
"SELECT N" in every case now?

Best regards,
Zoltán Böszörményi

-- 
Bible has answers for everything. Proof:
"But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
than these cometh of evil." (Matthew 5:37) - basics of digital technology.
"May your kingdom come" - superficial description of plate tectonics

----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/

  17. Re: [PATCH] Provide rowcount for utility SELECTs

    Robert Haas <robertmhaas@gmail.com> — 2010-02-01T20:42:02Z

    2010/1/12 Boszormenyi Zoltan <zb@cybertec.at>:
> Tom Lane írta:
>> Alvaro Herrera <alvherre@commandprompt.com> writes:
>>
>>> But it would be broken in very obvious ways, no?  It's not like it would
>>> be silently broken and thus escape testing ...
>>>
>>
>> Well, if we wanted to adopt that approach, we should add the count to
>> *all* SELECT tags not just a small subset.  As the patch stands it
>> seems entirely possible that a breakage would escape immediate notice.
>>
>
> Can you give me an example that would return
> plain "SELECT" after my new patch? I added
> one more change to the patch, is it enough to return
> "SELECT N" in every case now?

I just tested this, so I can say definitely: no.  I hacked psql with
the attached patch, and if you just do a plain old SELECT * FROM
table, you get back only SELECT, not SELECT N.

...Robert

  18. Re: [PATCH] Provide rowcount for utility SELECTs

    Boszormenyi Zoltan <zb@cybertec.at> — 2010-02-02T09:03:21Z

    Robert Haas írta:
> 2010/1/12 Boszormenyi Zoltan <zb@cybertec.at>:
>   
>> Tom Lane írta:
>>     
>>> Alvaro Herrera <alvherre@commandprompt.com> writes:
>>>
>>>       
>>>> But it would be broken in very obvious ways, no?  It's not like it would
>>>> be silently broken and thus escape testing ...
>>>>
>>>>         
>>> Well, if we wanted to adopt that approach, we should add the count to
>>> *all* SELECT tags not just a small subset.  As the patch stands it
>>> seems entirely possible that a breakage would escape immediate notice.
>>>
>>>       
>> Can you give me an example that would return
>> plain "SELECT" after my new patch? I added
>> one more change to the patch, is it enough to return
>> "SELECT N" in every case now?
>>     
>
> I just tested this, so I can say definitely: no.  I hacked psql with
> the attached patch, and if you just do a plain old SELECT * FROM
> table, you get back only SELECT, not SELECT N.
>
> ...Robert
>   

Thanks for testing it, with the attached patch your test case also
returns SELECT N.

Best regards,
Zoltán Böszörményi

-- 
Bible has answers for everything. Proof:
"But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
than these cometh of evil." (Matthew 5:37) - basics of digital technology.
"May your kingdom come" - superficial description of plate tectonics

----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/

  19. Re: [PATCH] Provide rowcount for utility SELECTs

    Robert Haas <robertmhaas@gmail.com> — 2010-02-07T05:01:08Z

    On Tue, Feb 2, 2010 at 4:03 AM, Boszormenyi Zoltan <zb@cybertec.at> wrote:
> Thanks for testing it, with the attached patch your test case also
> returns SELECT N.

Thoughts:

1. Looks like you've falsified the last comment block in PortalRunMulti().
2. I don't like the duplication of code in PortalRun() between the
first and second branches of the switch statement.
3. You've falsified the comment preceding that code, too.
4. Is there any reason to use pg_strcasecmp() instead of plain old strcmp()?

Someone who knows the overall structure of the code better than I do
will have to comment on whether there are any code paths to worry
about that do not go through PortalRun().

A general concern I have is that this what we're basically doing here
is handling the most common case in ProcessQuery() and then installing
fallback mechanisms to pick up any stragglers: but the fallback
mechanisms only guarantee that we'll add a number to the command tag,
not that it will be meaningful.  Unfortunately, my limited imagination
can't quite figure out in what cases we'll get a SELECT command tag
back other than (1) plain old SELECT, (2) SELECT INTO, and (3) CTAS,
so I'm not sure what to go test.

...Robert

  20. Re: [PATCH] Provide rowcount for utility SELECTs

    Boszormenyi Zoltan <zb@cybertec.at> — 2010-02-07T17:46:18Z

    Robert Haas írta:
> On Tue, Feb 2, 2010 at 4:03 AM, Boszormenyi Zoltan <zb@cybertec.at> wrote:
>   
>> Thanks for testing it, with the attached patch your test case also
>> returns SELECT N.
>>     
>
> Thoughts:
>
> 1. Looks like you've falsified the last comment block in PortalRunMulti().
>   

You mean the "fake something up" part? Will fix the comment.

> 2. I don't like the duplication of code in PortalRun() between the
> first and second branches of the switch statement.
>   

The PORTAL_ONE_SELECT and PORTAL_ONE_RETURNING/PORTAL_UTIL_SELECT
cases differ only in that the latter case runs this below everything else:
    if (!portal->holdStore)
        FillPortalStore(portal, isTopLevel);
Would it be desired to unify these cases? This way there would be
no code duplication. Something like:
    if (portal->strategy != PORTAL_ONE_SELECT && !portal->holdStore)
        FillPortalStore(portal, isTopLevel);
    ... (everything else)

> 3. You've falsified the comment preceding that code, too.
>   

This one?

        /*
         * Set up global portal context pointers.
         *
         * We have to play a special game here to support utility
commands like
         * VACUUM and CLUSTER, which internally start and commit
transactions.
         * When we are called to execute such a command,
CurrentResourceOwner will
         * be pointing to the TopTransactionResourceOwner --- which will be
         * destroyed and replaced in the course of the internal commit and
         * restart.  So we need to be prepared to restore it as pointing
to the
         * exit-time TopTransactionResourceOwner.  (Ain't that ugly? 
This idea of
         * internally starting whole new transactions is not good.)
         * CurrentMemoryContext has a similar problem, but the other
pointers we
         * save here will be NULL or pointing to longer-lived objects.
         */

I can't see why. Can you clarify?

Or this one?
                                /* we know the query is supposed to set
the tag */
                                if (completionTag && portal->commandTag)
                                  ...
The condition and the comment still seems to be true.

Which comment are you talking about?

> 4. Is there any reason to use pg_strcasecmp() instead of plain old strcmp()?
>   

I don't have any particular reason, strcmp() would do.

> Someone who knows the overall structure of the code better than I do
> will have to comment on whether there are any code paths to worry
> about that do not go through PortalRun().
>
> A general concern I have is that this what we're basically doing here
> is handling the most common case in ProcessQuery() and then installing
> fallback mechanisms to pick up any stragglers: but the fallback
> mechanisms only guarantee that we'll add a number to the command tag,
> not that it will be meaningful.  Unfortunately, my limited imagination
> can't quite figure out in what cases we'll get a SELECT command tag
> back other than (1) plain old SELECT, (2) SELECT INTO, and (3) CTAS,
> so I'm not sure what to go test.
>
> ...Robert
>
>   

Best regards,
Zoltán Böszörményi

-- 
Bible has answers for everything. Proof:
"But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
than these cometh of evil." (Matthew 5:37) - basics of digital technology.
"May your kingdom come" - superficial description of plate tectonics

----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/

  21. Re: [PATCH] Provide rowcount for utility SELECTs

    Robert Haas <robertmhaas@gmail.com> — 2010-02-08T04:17:53Z

    On Sun, Feb 7, 2010 at 12:46 PM, Boszormenyi Zoltan <zb@cybertec.at> wrote:
>> 1. Looks like you've falsified the last comment block in PortalRunMulti().
> You mean the "fake something up" part? Will fix the comment.

Yes.

>> 2. I don't like the duplication of code in PortalRun() between the
>> first and second branches of the switch statement.
> The PORTAL_ONE_SELECT and PORTAL_ONE_RETURNING/PORTAL_UTIL_SELECT
> cases differ only in that the latter case runs this below everything else:
>    if (!portal->holdStore)
>        FillPortalStore(portal, isTopLevel);
> Would it be desired to unify these cases? This way there would be
> no code duplication. Something like:
>    if (portal->strategy != PORTAL_ONE_SELECT && !portal->holdStore)
>        FillPortalStore(portal, isTopLevel);
>    ... (everything else)

I thought about that but wasn't sure.  I recommend that you give it a
try that way and we'll see how it looks.

>> 3. You've falsified the comment preceding that code, too.
>
> Or this one?
>                                /* we know the query is supposed to set
> the tag */
>                                if (completionTag && portal->commandTag)
>                                  ...
> The condition and the comment still seems to be true.

This is the one I was referring to.  I guess "falsified" was too
strong, but I don't think that comment describes the function of that
code too well any more.  Maybe it didn't before either, but I think
it's worse now.

>> 4. Is there any reason to use pg_strcasecmp() instead of plain old strcmp()?
> I don't have any particular reason, strcmp() would do.

OK, please change it.

>> Someone who knows the overall structure of the code better than I do
>> will have to comment on whether there are any code paths to worry
>> about that do not go through PortalRun().
>>
>> A general concern I have is that this what we're basically doing here
>> is handling the most common case in ProcessQuery() and then installing
>> fallback mechanisms to pick up any stragglers: but the fallback
>> mechanisms only guarantee that we'll add a number to the command tag,
>> not that it will be meaningful.  Unfortunately, my limited imagination
>> can't quite figure out in what cases we'll get a SELECT command tag
>> back other than (1) plain old SELECT, (2) SELECT INTO, and (3) CTAS,
>> so I'm not sure what to go test.

Any thoughts on these issues, anyone?

...Robert

  22. Re: [PATCH] Provide rowcount for utility SELECTs

    Boszormenyi Zoltan <zb@cybertec.at> — 2010-02-08T10:53:34Z

    Robert Haas írta:
> ...
> OK, please change it.
>   

New patch is attached with the discussed changes.

>>> Someone who knows the overall structure of the code better than I do
>>> will have to comment on whether there are any code paths to worry
>>> about that do not go through PortalRun().
>>>
>>> A general concern I have is that this what we're basically doing here
>>> is handling the most common case in ProcessQuery() and then installing
>>> fallback mechanisms to pick up any stragglers: but the fallback
>>> mechanisms only guarantee that we'll add a number to the command tag,
>>> not that it will be meaningful.  Unfortunately, my limited imagination
>>> can't quite figure out in what cases we'll get a SELECT command tag
>>> back other than (1) plain old SELECT, (2) SELECT INTO, and (3) CTAS,
>>> so I'm not sure what to go test.
>>>       
>
> Any thoughts on these issues, anyone?
>
> ...Robert
>   

Best regards,
Zoltán Böszörményi

-- 
Bible has answers for everything. Proof:
"But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
than these cometh of evil." (Matthew 5:37) - basics of digital technology.
"May your kingdom come" - superficial description of plate tectonics

----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/

  23. Re: [PATCH] Provide rowcount for utility SELECTs

    Robert Haas <robertmhaas@gmail.com> — 2010-02-11T18:52:12Z

    On Mon, Feb 8, 2010 at 5:53 AM, Boszormenyi Zoltan <zb@cybertec.at> wrote:
> New patch is attached with the discussed changes.

This looks OK to me now, but it lacks docs.

I'll set it to Waiting on Author.

...Robert

  24. Re: [PATCH] Provide rowcount for utility SELECTs

    Alvaro Herrera <alvherre@commandprompt.com> — 2010-02-11T19:04:08Z

    Boszormenyi Zoltan escribió:
> Robert Haas írta:
> > ...
> > OK, please change it.
> >   
> 
> New patch is attached with the discussed changes.

This looks all wrong.  PORTAL_ONE_SELECT is now being passed through
FillPortalStore, which runs it to completion, whereas it was previously
passed via PortalRunSelect first, which has different semantics
regarding the "count" arg.

Also, even if that weren't wrong, FillPortalStore states at its header
comment that it is only used for the other two cases (ONE_RETURNING and
UTIL_SELECT), but now is being used for ONE_SELECT as well.

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

  25. Re: [PATCH] Provide rowcount for utility SELECTs

    Robert Haas <robertmhaas@gmail.com> — 2010-02-11T19:25:48Z

    On Thu, Feb 11, 2010 at 2:04 PM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:
> Boszormenyi Zoltan escribió:
>> Robert Haas írta:
>> > ...
>> > OK, please change it.
>> >
>>
>> New patch is attached with the discussed changes.
>
> This looks all wrong.  PORTAL_ONE_SELECT is now being passed through
> FillPortalStore, which runs it to completion, whereas it was previously
> passed via PortalRunSelect first, which has different semantics
> regarding the "count" arg.
>
> Also, even if that weren't wrong, FillPortalStore states at its header
> comment that it is only used for the other two cases (ONE_RETURNING and
> UTIL_SELECT), but now is being used for ONE_SELECT as well.

I was all prepared to admit that I hadn't actually looked at the patch
carefully enough, but I just looked at (and CVS HEAD) again and what
you've written here doesn't appear to describe what I'm seeing in the
code:

				if ((portal->strategy != PORTAL_ONE_SELECT) && (!portal->holdStore))
					FillPortalStore(portal, isTopLevel);

So one of us is confused... it may well be me.

...Robert

  26. Re: [PATCH] Provide rowcount for utility SELECTs

    Alvaro Herrera <alvherre@commandprompt.com> — 2010-02-11T19:38:53Z

    Robert Haas escribió:

> I was all prepared to admit that I hadn't actually looked at the patch
> carefully enough, but I just looked at (and CVS HEAD) again and what
> you've written here doesn't appear to describe what I'm seeing in the
> code:
> 
> 				if ((portal->strategy != PORTAL_ONE_SELECT) && (!portal->holdStore))
> 					FillPortalStore(portal, isTopLevel);
> 
> So one of us is confused... it may well be me.

Ah, it seems I misread it ... but then I don't quite see the point in
that change.

Well, not doing a full review anyway, so never mind me.

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

  27. Re: [PATCH] Provide rowcount for utility SELECTs

    Robert Haas <robertmhaas@gmail.com> — 2010-02-11T19:42:49Z

    On Thu, Feb 11, 2010 at 2:38 PM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:
> Robert Haas escribió:
>
>> I was all prepared to admit that I hadn't actually looked at the patch
>> carefully enough, but I just looked at (and CVS HEAD) again and what
>> you've written here doesn't appear to describe what I'm seeing in the
>> code:
>>
>>                               if ((portal->strategy != PORTAL_ONE_SELECT) && (!portal->holdStore))
>>                                       FillPortalStore(portal, isTopLevel);
>>
>> So one of us is confused... it may well be me.
>
> Ah, it seems I misread it ... but then I don't quite see the point in
> that change.

Well the point is just that Zoltan is adding some more code that
applies to both branches of the switch, so merging them saves some
duplication.

> Well, not doing a full review anyway, so never mind me.

Actually I was sort of hoping you (or someone other than me) would
pick this up for commit...

...Robert

  28. Re: [PATCH] Provide rowcount for utility SELECTs

    Alvaro Herrera <alvherre@commandprompt.com> — 2010-02-11T19:47:26Z

    Robert Haas escribió:
> On Thu, Feb 11, 2010 at 2:38 PM, Alvaro Herrera
> <alvherre@commandprompt.com> wrote:
> > Robert Haas escribió:
> >
> >> I was all prepared to admit that I hadn't actually looked at the patch
> >> carefully enough, but I just looked at (and CVS HEAD) again and what
> >> you've written here doesn't appear to describe what I'm seeing in the
> >> code:
> >>
> >>                               if ((portal->strategy != PORTAL_ONE_SELECT) && (!portal->holdStore))
> >>                                       FillPortalStore(portal, isTopLevel);
> >>
> >> So one of us is confused... it may well be me.
> >
> > Ah, it seems I misread it ... but then I don't quite see the point in
> > that change.
> 
> Well the point is just that Zoltan is adding some more code that
> applies to both branches of the switch, so merging them saves some
> duplication.

But then there's no other branches, so why not just put it below the
switch?

> > Well, not doing a full review anyway, so never mind me.
> 
> Actually I was sort of hoping you (or someone other than me) would
> pick this up for commit...

Hmm ...

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

  29. Re: [PATCH] Provide rowcount for utility SELECTs

    Robert Haas <robertmhaas@gmail.com> — 2010-02-11T19:50:42Z

    On Thu, Feb 11, 2010 at 2:47 PM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:
> Robert Haas escribió:
>> On Thu, Feb 11, 2010 at 2:38 PM, Alvaro Herrera
>> <alvherre@commandprompt.com> wrote:
>> > Robert Haas escribió:
>> >
>> >> I was all prepared to admit that I hadn't actually looked at the patch
>> >> carefully enough, but I just looked at (and CVS HEAD) again and what
>> >> you've written here doesn't appear to describe what I'm seeing in the
>> >> code:
>> >>
>> >>                               if ((portal->strategy != PORTAL_ONE_SELECT) && (!portal->holdStore))
>> >>                                       FillPortalStore(portal, isTopLevel);
>> >>
>> >> So one of us is confused... it may well be me.
>> >
>> > Ah, it seems I misread it ... but then I don't quite see the point in
>> > that change.
>>
>> Well the point is just that Zoltan is adding some more code that
>> applies to both branches of the switch, so merging them saves some
>> duplication.
>
> But then there's no other branches, so why not just put it below the
> switch?

No, PORTAL_MULTI_QUERY is still separate.

>> > Well, not doing a full review anyway, so never mind me.
>>
>> Actually I was sort of hoping you (or someone other than me) would
>> pick this up for commit...
>
> Hmm ...

Maybe I came to the wrong place.  :-)

...Robert

  30. Re: [PATCH] Provide rowcount for utility SELECTs

    Boszormenyi Zoltan <zb@cybertec.at> — 2010-02-11T20:00:56Z

    Alvaro Herrera írta:
> Boszormenyi Zoltan escribió:
>   
>> Robert Haas írta:
>>     
>>> ...
>>> OK, please change it.
>>>   
>>>       
>> New patch is attached with the discussed changes.
>>     
>
> This looks all wrong.  PORTAL_ONE_SELECT is now being passed through
> FillPortalStore,

Where do you read that? The code in my patch reads:

                                /*
                                 * If we have not yet run the command,
do so, storing its
!                                * results in the portal's tuplestore.
Do this only for the
!                                * PORTAL_ONE_RETURNING and
PORTAL_UTIL_SELECT cases.
                                 */
!                               if ((portal->strategy !=
PORTAL_ONE_SELECT) && (!portal->holdStore))
                                        FillPortalStore(portal, isTopLevel);

So, PORTAL_ONE_SELECT doesn't run through FillPortalStore().

>  which runs it to completion, whereas it was previously
> passed via PortalRunSelect first, which has different semantics
> regarding the "count" arg.
>
> Also, even if that weren't wrong, FillPortalStore states at its header
> comment that it is only used for the other two cases (ONE_RETURNING and
> UTIL_SELECT), but now is being used for ONE_SELECT as well.
>
>   


-- 
Bible has answers for everything. Proof:
"But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
than these cometh of evil." (Matthew 5:37) - basics of digital technology.
"May your kingdom come" - superficial description of plate tectonics

----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/

  31. Re: [PATCH] Provide rowcount for utility SELECTs

    Boszormenyi Zoltan <zb@cybertec.at> — 2010-02-11T20:09:06Z

    Alvaro Herrera írta:
> Robert Haas escribió:
>   
>> On Thu, Feb 11, 2010 at 2:38 PM, Alvaro Herrera
>> <alvherre@commandprompt.com> wrote:
>>     
>>> Robert Haas escribió:
>>>
>>>       
>>>> I was all prepared to admit that I hadn't actually looked at the patch
>>>> carefully enough, but I just looked at (and CVS HEAD) again and what
>>>> you've written here doesn't appear to describe what I'm seeing in the
>>>> code:
>>>>
>>>>                               if ((portal->strategy != PORTAL_ONE_SELECT) && (!portal->holdStore))
>>>>                                       FillPortalStore(portal, isTopLevel);
>>>>
>>>> So one of us is confused... it may well be me.
>>>>         
>>> Ah, it seems I misread it ... but then I don't quite see the point in
>>> that change.
>>>       
>> Well the point is just that Zoltan is adding some more code that
>> applies to both branches of the switch, so merging them saves some
>> duplication.
>>     
>
> But then there's no other branches, so why not just put it below the
> switch?
>   

Well, for one because the PORTAL_MULTI_QUERY case doesn't
set the command tag even there was one from the last one and the
caller provided the non-NULL pointer. That would be behaviour change.

>>> Well, not doing a full review anyway, so never mind me.
>>>       
>> Actually I was sort of hoping you (or someone other than me) would
>> pick this up for commit...
>>     
>
> Hmm ...
>
>   


-- 
Bible has answers for everything. Proof:
"But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
than these cometh of evil." (Matthew 5:37) - basics of digital technology.
"May your kingdom come" - superficial description of plate tectonics

----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/

  32. Re: [PATCH] Provide rowcount for utility SELECTs

    Boszormenyi Zoltan <zb@cybertec.at> — 2010-02-12T15:48:32Z

    Robert Haas írta:
> On Mon, Feb 8, 2010 at 5:53 AM, Boszormenyi Zoltan <zb@cybertec.at> wrote:
>   
>> New patch is attached with the discussed changes.
>>     
>
> This looks OK to me now, but it lacks docs.
>
> I'll set it to Waiting on Author.
>
> ...Robert
>   

I added a small change to protocol.sgml for the
"CommandComplete" message describing the change.
Is it enough?

Best regards,
Zoltán Böszörményi

-- 
Bible has answers for everything. Proof:
"But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
than these cometh of evil." (Matthew 5:37) - basics of digital technology.
"May your kingdom come" - superficial description of plate tectonics

----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/

  33. Re: [PATCH] Provide rowcount for utility SELECTs

    Robert Haas <robertmhaas@gmail.com> — 2010-02-12T17:34:21Z

    On Fri, Feb 12, 2010 at 10:48 AM, Boszormenyi Zoltan <zb@cybertec.at> wrote:
> I added a small change to protocol.sgml for the
> "CommandComplete" message describing the change.
> Is it enough?

Ah, I didn't even see that that section needed to be updated.  Good
catch.  I'd suggest the following wording:

For a <command>SELECT</command> or <command>CREATE TABLE AS</command>
command, the tag is SELECT rows... [and the rest as you have it]

We should probably also retitle that section from "Retrieving Result
Information for Other Commands" to "Retrieving Other Result
Information" and adjust the text of the opening sentence similarly.

Also I think you need to update the docs for PQcmdtuples to mention
that it not works for SELECT and CTAS.

...Robert

  34. Re: [PATCH] Provide rowcount for utility SELECTs

    Boszormenyi Zoltan <zb@cybertec.at> — 2010-02-12T18:22:06Z

    Robert Haas írta:
> On Fri, Feb 12, 2010 at 10:48 AM, Boszormenyi Zoltan <zb@cybertec.at> wrote:
>   
>> I added a small change to protocol.sgml for the
>> "CommandComplete" message describing the change.
>> Is it enough?
>>     
>
> Ah, I didn't even see that that section needed to be updated.  Good
> catch.  I'd suggest the following wording:
>
> For a <command>SELECT</command> or <command>CREATE TABLE AS</command>
> command, the tag is SELECT rows... [and the rest as you have it]
>
> We should probably also retitle that section from "Retrieving Result
> Information for Other Commands" to "Retrieving Other Result
> Information" and adjust the text of the opening sentence similarly.
>
> Also I think you need to update the docs for PQcmdtuples to mention
> that it not works for SELECT and CTAS.
>   

Ok, I will update libpq.sgml where this section resides.
What's a CTA, btw? Do you mean CTE, a.k.a. "Common Table Expression"?

Thanks,
Zoltán Böszörményi

-- 
Bible has answers for everything. Proof:
"But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
than these cometh of evil." (Matthew 5:37) - basics of digital technology.
"May your kingdom come" - superficial description of plate tectonics

----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/

  35. Re: [PATCH] Provide rowcount for utility SELECTs

    Boszormenyi Zoltan <zb@cybertec.at> — 2010-02-12T18:29:08Z

    Robert Haas írta:
> On Fri, Feb 12, 2010 at 10:48 AM, Boszormenyi Zoltan <zb@cybertec.at> wrote:
>   
>> I added a small change to protocol.sgml for the
>> "CommandComplete" message describing the change.
>> Is it enough?
>>     
>
> Ah, I didn't even see that that section needed to be updated.  Good
> catch.  I'd suggest the following wording:
>
> For a <command>SELECT</command> or <command>CREATE TABLE AS</command>
> command, the tag is SELECT rows... [and the rest as you have it]
>
> We should probably also retitle that section from "Retrieving Result
> Information for Other Commands" to "Retrieving Other Result
> Information" and adjust the text of the opening sentence similarly.
>
> Also I think you need to update the docs for PQcmdtuples to mention
> that it not works for SELECT and CTAS.
>   

I mentioned the "WITH" command, instead of CTEs.

Best regards,
Zoltán Böszörményi

-- 
Bible has answers for everything. Proof:
"But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
than these cometh of evil." (Matthew 5:37) - basics of digital technology.
"May your kingdom come" - superficial description of plate tectonics

----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/

  36. Re: [PATCH] Provide rowcount for utility SELECTs

    Robert Haas <robertmhaas@gmail.com> — 2010-02-12T18:38:11Z

    On Fri, Feb 12, 2010 at 1:22 PM, Boszormenyi Zoltan <zb@cybertec.at> wrote:
> Robert Haas írta:
>> On Fri, Feb 12, 2010 at 10:48 AM, Boszormenyi Zoltan <zb@cybertec.at> wrote:
>>
>>> I added a small change to protocol.sgml for the
>>> "CommandComplete" message describing the change.
>>> Is it enough?
>>>
>>
>> Ah, I didn't even see that that section needed to be updated.  Good
>> catch.  I'd suggest the following wording:
>>
>> For a <command>SELECT</command> or <command>CREATE TABLE AS</command>
>> command, the tag is SELECT rows... [and the rest as you have it]
>>
>> We should probably also retitle that section from "Retrieving Result
>> Information for Other Commands" to "Retrieving Other Result
>> Information" and adjust the text of the opening sentence similarly.
>>
>> Also I think you need to update the docs for PQcmdtuples to mention
>> that it not works for SELECT and CTAS.
>>
>
> Ok, I will update libpq.sgml where this section resides.
> What's a CTA, btw? Do you mean CTE, a.k.a. "Common Table Expression"?

Sorry, CTAS = CREATE TABLE AS SELECT.

...Robert

  37. Re: [PATCH] Provide rowcount for utility SELECTs

    Boszormenyi Zoltan <zb@cybertec.at> — 2010-02-12T18:55:36Z

    Robert Haas írta:
> On Fri, Feb 12, 2010 at 1:22 PM, Boszormenyi Zoltan <zb@cybertec.at> wrote:
>   
>> Robert Haas írta:
>>     
>>> On Fri, Feb 12, 2010 at 10:48 AM, Boszormenyi Zoltan <zb@cybertec.at> wrote:
>>>
>>>       
>>>> I added a small change to protocol.sgml for the
>>>> "CommandComplete" message describing the change.
>>>> Is it enough?
>>>>
>>>>         
>>> Ah, I didn't even see that that section needed to be updated.  Good
>>> catch.  I'd suggest the following wording:
>>>
>>> For a <command>SELECT</command> or <command>CREATE TABLE AS</command>
>>> command, the tag is SELECT rows... [and the rest as you have it]
>>>
>>> We should probably also retitle that section from "Retrieving Result
>>> Information for Other Commands" to "Retrieving Other Result
>>> Information" and adjust the text of the opening sentence similarly.
>>>
>>> Also I think you need to update the docs for PQcmdtuples to mention
>>> that it not works for SELECT and CTAS.
>>>
>>>       
>> Ok, I will update libpq.sgml where this section resides.
>> What's a CTA, btw? Do you mean CTE, a.k.a. "Common Table Expression"?
>>     
>
> Sorry, CTAS = CREATE TABLE AS SELECT.
>   

Okay, new patch is attached. Please read the docs changes, and comment.

Thanks,
Zoltán Böszörményi

-- 
Bible has answers for everything. Proof:
"But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
than these cometh of evil." (Matthew 5:37) - basics of digital technology.
"May your kingdom come" - superficial description of plate tectonics

----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/

  38. Re: [PATCH] Provide rowcount for utility SELECTs

    Robert Haas <robertmhaas@gmail.com> — 2010-02-12T20:06:16Z

    On Fri, Feb 12, 2010 at 1:55 PM, Boszormenyi Zoltan <zb@cybertec.at> wrote:
> Okay, new patch is attached. Please read the docs changes, and comment.

I like it.  I'll mark it as Ready for Committer.

...Robert

  39. Re: [PATCH] Provide rowcount for utility SELECTs

    Bruce Momjian <bruce@momjian.us> — 2010-02-14T03:15:26Z

    Boszormenyi Zoltan wrote:
> >>> Ah, I didn't even see that that section needed to be updated.  Good
> >>> catch.  I'd suggest the following wording:
> >>>
> >>> For a <command>SELECT</command> or <command>CREATE TABLE AS</command>
> >>> command, the tag is SELECT rows... [and the rest as you have it]
> >>>
> >>> We should probably also retitle that section from "Retrieving Result
> >>> Information for Other Commands" to "Retrieving Other Result
> >>> Information" and adjust the text of the opening sentence similarly.
> >>>
> >>> Also I think you need to update the docs for PQcmdtuples to mention
> >>> that it not works for SELECT and CTAS.
> >>>
> >>>       
> >> Ok, I will update libpq.sgml where this section resides.
> >> What's a CTA, btw? Do you mean CTE, a.k.a. "Common Table Expression"?
> >>     
> >
> > Sorry, CTAS = CREATE TABLE AS SELECT.
> >   
> 
> Okay, new patch is attached. Please read the docs changes, and comment.

I have reviewed this patch and made some adjustments, attached.  The
major change is that our return of "0 0" in certain cases must remain,
though I have improved the C comment explaining it with a separate CVS
commit:

    /*
     * If a command completion tag was supplied, use it.  Otherwise use the
     * portal's commandTag as the default completion tag.
     *
     * Exception: Clients expect INSERT/UPDATE/DELETE tags to have
     * counts, so fake them with zeros.  This can happen with DO INSTEAD
     * rules if there is no replacement query of the same type as the
     * original.  We print "0 0" here because technically there is no
     * query of the matching tag type, and printing a non-zero count for
     * a different query type seems wrong, e.g.  an INSERT that does
     * an UPDATE instead should not print "0 1" if one row
     * was updated.  See QueryRewrite(), step 3, for details.
     */

I have removed the part of the patch that chagned "0 0";  it seems to
run fine without it.  The rest of my adjustments were minor.

One major part of the patch seems to be the collection of the
PORTAL_ONE_SELECT switch label into the label below it, which is a nice
cleanup.

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + If your life is a hard drive, Christ can be your backup. +

  40. Re: [PATCH] Provide rowcount for utility SELECTs

    Robert Haas <robertmhaas@gmail.com> — 2010-02-14T03:30:24Z

    On Sat, Feb 13, 2010 at 10:15 PM, Bruce Momjian <bruce@momjian.us> wrote:
> Boszormenyi Zoltan wrote:
>> >>> Ah, I didn't even see that that section needed to be updated.  Good
>> >>> catch.  I'd suggest the following wording:
>> >>>
>> >>> For a <command>SELECT</command> or <command>CREATE TABLE AS</command>
>> >>> command, the tag is SELECT rows... [and the rest as you have it]
>> >>>
>> >>> We should probably also retitle that section from "Retrieving Result
>> >>> Information for Other Commands" to "Retrieving Other Result
>> >>> Information" and adjust the text of the opening sentence similarly.
>> >>>
>> >>> Also I think you need to update the docs for PQcmdtuples to mention
>> >>> that it not works for SELECT and CTAS.
>> >>>
>> >>>
>> >> Ok, I will update libpq.sgml where this section resides.
>> >> What's a CTA, btw? Do you mean CTE, a.k.a. "Common Table Expression"?
>> >>
>> >
>> > Sorry, CTAS = CREATE TABLE AS SELECT.
>> >
>>
>> Okay, new patch is attached. Please read the docs changes, and comment.
>
> I have reviewed this patch and made some adjustments, attached.  The
> major change is that our return of "0 0" in certain cases must remain,
> though I have improved the C comment explaining it with a separate CVS
> commit:
>
>    /*
>     * If a command completion tag was supplied, use it.  Otherwise use the
>     * portal's commandTag as the default completion tag.
>     *
>     * Exception: Clients expect INSERT/UPDATE/DELETE tags to have
>     * counts, so fake them with zeros.  This can happen with DO INSTEAD
>     * rules if there is no replacement query of the same type as the
>     * original.  We print "0 0" here because technically there is no
>     * query of the matching tag type, and printing a non-zero count for
>     * a different query type seems wrong, e.g.  an INSERT that does
>     * an UPDATE instead should not print "0 1" if one row
>     * was updated.  See QueryRewrite(), step 3, for details.
>     */
>
> I have removed the part of the patch that chagned "0 0";  it seems to
> run fine without it.  The rest of my adjustments were minor.
>
> One major part of the patch seems to be the collection of the
> PORTAL_ONE_SELECT switch label into the label below it, which is a nice
> cleanup.

Thanks for taking the time to review this.  I think your adjustments are good.

...Robert

  41. Re: [PATCH] Provide rowcount for utility SELECTs

    Bruce Momjian <bruce@momjian.us> — 2010-02-16T20:58:23Z

    Applied.  Thanks.

---------------------------------------------------------------------------

Bruce Momjian wrote:
> Boszormenyi Zoltan wrote:
> > >>> Ah, I didn't even see that that section needed to be updated.  Good
> > >>> catch.  I'd suggest the following wording:
> > >>>
> > >>> For a <command>SELECT</command> or <command>CREATE TABLE AS</command>
> > >>> command, the tag is SELECT rows... [and the rest as you have it]
> > >>>
> > >>> We should probably also retitle that section from "Retrieving Result
> > >>> Information for Other Commands" to "Retrieving Other Result
> > >>> Information" and adjust the text of the opening sentence similarly.
> > >>>
> > >>> Also I think you need to update the docs for PQcmdtuples to mention
> > >>> that it not works for SELECT and CTAS.
> > >>>
> > >>>       
> > >> Ok, I will update libpq.sgml where this section resides.
> > >> What's a CTA, btw? Do you mean CTE, a.k.a. "Common Table Expression"?
> > >>     
> > >
> > > Sorry, CTAS = CREATE TABLE AS SELECT.
> > >   
> > 
> > Okay, new patch is attached. Please read the docs changes, and comment.
> 
> I have reviewed this patch and made some adjustments, attached.  The
> major change is that our return of "0 0" in certain cases must remain,
> though I have improved the C comment explaining it with a separate CVS
> commit:
> 
>     /*
>      * If a command completion tag was supplied, use it.  Otherwise use the
>      * portal's commandTag as the default completion tag.
>      *
>      * Exception: Clients expect INSERT/UPDATE/DELETE tags to have
>      * counts, so fake them with zeros.  This can happen with DO INSTEAD
>      * rules if there is no replacement query of the same type as the
>      * original.  We print "0 0" here because technically there is no
>      * query of the matching tag type, and printing a non-zero count for
>      * a different query type seems wrong, e.g.  an INSERT that does
>      * an UPDATE instead should not print "0 1" if one row
>      * was updated.  See QueryRewrite(), step 3, for details.
>      */
> 
> I have removed the part of the patch that chagned "0 0";  it seems to
> run fine without it.  The rest of my adjustments were minor.
> 
> One major part of the patch seems to be the collection of the
> PORTAL_ONE_SELECT switch label into the label below it, which is a nice
> cleanup.
> 
> -- 
>   Bruce Momjian  <bruce@momjian.us>        http://momjian.us
>   EnterpriseDB                             http://enterprisedb.com
> 
>   + If your life is a hard drive, Christ can be your backup. +

> Index: doc/src/sgml/libpq.sgml
> ===================================================================
> RCS file: /cvsroot/pgsql/doc/src/sgml/libpq.sgml,v
> retrieving revision 1.297
> diff -c -c -r1.297 libpq.sgml
> *** doc/src/sgml/libpq.sgml	5 Feb 2010 03:09:04 -0000	1.297
> --- doc/src/sgml/libpq.sgml	14 Feb 2010 03:11:00 -0000
> ***************
> *** 2869,2880 ****
>     </sect2>
>   
>     <sect2 id="libpq-exec-nonselect">
> !    <title>Retrieving Result Information for Other Commands</title>
>   
>      <para>
> !     These functions are used to extract information from
> !     <structname>PGresult</structname> objects that are not
> !     <command>SELECT</> results.
>      </para>
>   
>      <variablelist>
> --- 2869,2879 ----
>     </sect2>
>   
>     <sect2 id="libpq-exec-nonselect">
> !    <title>Retrieving Other Result Information</title>
>   
>      <para>
> !     These functions are used to extract other information from
> !     <structname>PGresult</structname> objects.
>      </para>
>   
>      <variablelist>
> ***************
> *** 2925,2936 ****
>          This function returns a string containing the number of rows
>          affected by the <acronym>SQL</> statement that generated the
>          <structname>PGresult</>. This function can only be used following
> !        the execution of an <command>INSERT</>, <command>UPDATE</>,
> !        <command>DELETE</>, <command>MOVE</>, <command>FETCH</>, or
> !        <command>COPY</> statement, or an <command>EXECUTE</> of a
> !        prepared query that contains an <command>INSERT</>,
> !        <command>UPDATE</>, or <command>DELETE</> statement.  If the
> !        command that generated the <structname>PGresult</> was anything
>          else, <function>PQcmdTuples</> returns an empty string. The caller
>          should not free the return value directly. It will be freed when
>          the associated <structname>PGresult</> handle is passed to
> --- 2924,2935 ----
>          This function returns a string containing the number of rows
>          affected by the <acronym>SQL</> statement that generated the
>          <structname>PGresult</>. This function can only be used following
> !        the execution of a <command>SELECT</>, <command>CREATE TABLE AS</>,
> !        <command>INSERT</>, <command>UPDATE</>, <command>DELETE</>,
> !        <command>MOVE</>, <command>FETCH</>, or <command>COPY</> statement,
> !        or an <command>EXECUTE</> of a prepared query that contains an
> !        <command>INSERT</>, <command>UPDATE</>, or <command>DELETE</> statement.
> !        If the command that generated the <structname>PGresult</> was anything
>          else, <function>PQcmdTuples</> returns an empty string. The caller
>          should not free the return value directly. It will be freed when
>          the associated <structname>PGresult</> handle is passed to
> Index: doc/src/sgml/protocol.sgml
> ===================================================================
> RCS file: /cvsroot/pgsql/doc/src/sgml/protocol.sgml,v
> retrieving revision 1.78
> diff -c -c -r1.78 protocol.sgml
> *** doc/src/sgml/protocol.sgml	3 Feb 2010 09:47:19 -0000	1.78
> --- doc/src/sgml/protocol.sgml	14 Feb 2010 03:11:00 -0000
> ***************
> *** 2222,2227 ****
> --- 2222,2233 ----
>          </para>
>   
>          <para>
> +         For a <command>SELECT</command> or <command>CREATE TABLE AS</command>
> +         command, the tag is <literal>SELECT <replaceable>rows</replaceable></literal>
> +         where <replaceable>rows</replaceable> is the number of rows retrieved.
> +        </para>
> + 
> +        <para>
>           For a <command>MOVE</command> command, the tag is
>           <literal>MOVE <replaceable>rows</replaceable></literal> where
>           <replaceable>rows</replaceable> is the number of rows the
> Index: src/backend/tcop/pquery.c
> ===================================================================
> RCS file: /cvsroot/pgsql/src/backend/tcop/pquery.c,v
> retrieving revision 1.135
> diff -c -c -r1.135 pquery.c
> *** src/backend/tcop/pquery.c	13 Feb 2010 22:45:41 -0000	1.135
> --- src/backend/tcop/pquery.c	14 Feb 2010 03:11:04 -0000
> ***************
> *** 205,211 ****
>   		switch (queryDesc->operation)
>   		{
>   			case CMD_SELECT:
> ! 				strcpy(completionTag, "SELECT");
>   				break;
>   			case CMD_INSERT:
>   				if (queryDesc->estate->es_processed == 1)
> --- 205,212 ----
>   		switch (queryDesc->operation)
>   		{
>   			case CMD_SELECT:
> ! 				snprintf(completionTag, COMPLETION_TAG_BUFSIZE,
> ! 						 "SELECT %u", queryDesc->estate->es_processed);
>   				break;
>   			case CMD_INSERT:
>   				if (queryDesc->estate->es_processed == 1)
> ***************
> *** 714,719 ****
> --- 715,721 ----
>   		  char *completionTag)
>   {
>   	bool		result;
> + 	uint32		nprocessed;
>   	ResourceOwner saveTopTransactionResourceOwner;
>   	MemoryContext saveTopTransactionContext;
>   	Portal		saveActivePortal;
> ***************
> *** 776,814 ****
>   		switch (portal->strategy)
>   		{
>   			case PORTAL_ONE_SELECT:
> - 				(void) PortalRunSelect(portal, true, count, dest);
> - 
> - 				/* we know the query is supposed to set the tag */
> - 				if (completionTag && portal->commandTag)
> - 					strcpy(completionTag, portal->commandTag);
> - 
> - 				/* Mark portal not active */
> - 				portal->status = PORTAL_READY;
> - 
> - 				/*
> - 				 * Since it's a forward fetch, say DONE iff atEnd is now true.
> - 				 */
> - 				result = portal->atEnd;
> - 				break;
> - 
>   			case PORTAL_ONE_RETURNING:
>   			case PORTAL_UTIL_SELECT:
>   
>   				/*
>   				 * If we have not yet run the command, do so, storing its
> ! 				 * results in the portal's tuplestore.
>   				 */
> ! 				if (!portal->holdStore)
>   					FillPortalStore(portal, isTopLevel);
>   
>   				/*
>   				 * Now fetch desired portion of results.
>   				 */
> ! 				(void) PortalRunSelect(portal, true, count, dest);
>   
> ! 				/* we know the query is supposed to set the tag */
>   				if (completionTag && portal->commandTag)
> ! 					strcpy(completionTag, portal->commandTag);
>   
>   				/* Mark portal not active */
>   				portal->status = PORTAL_READY;
> --- 778,812 ----
>   		switch (portal->strategy)
>   		{
>   			case PORTAL_ONE_SELECT:
>   			case PORTAL_ONE_RETURNING:
>   			case PORTAL_UTIL_SELECT:
>   
>   				/*
>   				 * If we have not yet run the command, do so, storing its
> ! 				 * results in the portal's tuplestore. Do this only for the
> ! 				 * PORTAL_ONE_RETURNING and PORTAL_UTIL_SELECT cases.
>   				 */
> ! 				if (portal->strategy != PORTAL_ONE_SELECT && !portal->holdStore)
>   					FillPortalStore(portal, isTopLevel);
>   
>   				/*
>   				 * Now fetch desired portion of results.
>   				 */
> ! 				nprocessed = PortalRunSelect(portal, true, count, dest);
>   
> ! 				/*
> ! 				 * If the portal result contains a command tag and the caller
> ! 				 * gave us a pointer to store it, copy it. Patch the "SELECT"
> ! 				 * tag to also provide the rowcount.
> ! 				 */
>   				if (completionTag && portal->commandTag)
> ! 				{
> ! 					if (strcmp(portal->commandTag, "SELECT") == 0)
> ! 						snprintf(completionTag, COMPLETION_TAG_BUFSIZE,
> ! 										"SELECT %u", nprocessed);
> ! 					else
> ! 						strcpy(completionTag, portal->commandTag);
> ! 				}
>   
>   				/* Mark portal not active */
>   				portal->status = PORTAL_READY;
> ***************
> *** 1331,1337 ****
>   	{
>   		if (portal->commandTag)
>   			strcpy(completionTag, portal->commandTag);
> ! 		if (strcmp(completionTag, "INSERT") == 0)
>   			strcpy(completionTag, "INSERT 0 0");
>   		else if (strcmp(completionTag, "UPDATE") == 0)
>   			strcpy(completionTag, "UPDATE 0");
> --- 1329,1337 ----
>   	{
>   		if (portal->commandTag)
>   			strcpy(completionTag, portal->commandTag);
> ! 		if (strcmp(completionTag, "SELECT") == 0)
> ! 			sprintf(completionTag, "SELECT 0 0");
> ! 		else if (strcmp(completionTag, "INSERT") == 0)
>   			strcpy(completionTag, "INSERT 0 0");
>   		else if (strcmp(completionTag, "UPDATE") == 0)
>   			strcpy(completionTag, "UPDATE 0");
> Index: src/interfaces/libpq/fe-exec.c
> ===================================================================
> RCS file: /cvsroot/pgsql/src/interfaces/libpq/fe-exec.c,v
> retrieving revision 1.208
> diff -c -c -r1.208 fe-exec.c
> *** src/interfaces/libpq/fe-exec.c	21 Jan 2010 18:43:25 -0000	1.208
> --- src/interfaces/libpq/fe-exec.c	14 Feb 2010 03:11:04 -0000
> ***************
> *** 2752,2758 ****
>   			goto interpret_error;		/* no space? */
>   		p++;
>   	}
> ! 	else if (strncmp(res->cmdStatus, "DELETE ", 7) == 0 ||
>   			 strncmp(res->cmdStatus, "UPDATE ", 7) == 0)
>   		p = res->cmdStatus + 7;
>   	else if (strncmp(res->cmdStatus, "FETCH ", 6) == 0)
> --- 2752,2759 ----
>   			goto interpret_error;		/* no space? */
>   		p++;
>   	}
> ! 	else if (strncmp(res->cmdStatus, "SELECT ", 7) == 0 ||
> ! 			 strncmp(res->cmdStatus, "DELETE ", 7) == 0 ||
>   			 strncmp(res->cmdStatus, "UPDATE ", 7) == 0)
>   		p = res->cmdStatus + 7;
>   	else if (strncmp(res->cmdStatus, "FETCH ", 6) == 0)

> 
> -- 
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + If your life is a hard drive, Christ can be your backup. +