Thread: [PATCH] Provide rowcount for utility SELECTs
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/
Attachment
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 > >
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
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/
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
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?
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
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
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
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/
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
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/
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/
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
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
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
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/
Attachment
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
Attachment
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/
Attachment
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
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
supportutility
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
andreplaced 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
similarproblem, 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/
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
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/
Attachment
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
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
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
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
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
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
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
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/
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/
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/
Attachment
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
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/
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/
Attachment
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
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/
Attachment
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
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)
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
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. +