Re: [PATCH] Provide rowcount for utility SELECTs

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/