Re: [PATCH] Provide rowcount for utility SELECTs

On Sun, Feb 7, 2010 at 12:46 PM, Boszormenyi Zoltan <zb(at)cybertec(dot)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