[PATCH] Provide rowcount for utility SELECTs

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

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/

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

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/

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