From: | Robert Haas <robertmhaas(at)gmail(dot)com> |
---|---|
To: | Boszormenyi Zoltan <zb(at)cybertec(dot)at> |
Cc: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Alvaro Herrera <alvherre(at)commandprompt(dot)com>, Peter Eisentraut <peter_e(at)gmx(dot)net>, Hans-Juergen Schoenig <hs(at)cybertec(dot)at>, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: [PATCH] Provide rowcount for utility SELECTs |
Date: | 2010-02-08 04:17:53 |
Message-ID: | 603c8f071002072017x6bb8285ekcd9941c9d83aef7c@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | Robert Haas | 2010-02-08 04:23:18 | Re: damage control mode |
Previous Message | Robert Haas | 2010-02-08 04:11:55 | Re: damage control mode |