On Sun, Feb 7, 2010 at 12:46 PM, Boszormenyi Zoltan <z...@cybertec.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
