Re: [PATCH] Provide rowcount for utility SELECTs

Robert Haas írta:
> On Tue, Feb 2, 2010 at 4:03 AM, Boszormenyi Zoltan <zb(at)cybertec(dot)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/