> On Feb 3, 2021, at 2:03 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Tue, Feb 2, 2021 at 6:10 PM Mark Dilger <mark.dilger@enterprisedb.com> wrote:
>> 0001 -- no changes
>
> Committed.
Thanks!
>> 0002 -- fixing omissions in @pgfeutilsfiles in file src/tools/msvc/Mkvcbuild.pm
Numbered 0001 in this next patch set.
> Here are a few minor cosmetic issues with this patch:
>
> - connect_utils.c lacks a file header comment.
Fixed
> - Some or perhaps all of the other file header comments need an update for 2021.
Fixed.
> - There's bogus hunks in the diff for string_utils.c.
Removed.
> I think the rest of this looks good. I spent a long time puzzling over
> whether consumeQueryResult() and processQueryResult() needed to be
> moved, but then I realized that this patch actually makes them into
> static functions inside parallel_slot.c, rather than public functions
> as they were before. I like that. The only reason those functions need
> to be moved at all is so that the scripts_parallel/parallel_slot stuff
> can continue to do its thing, so this is actually a better way of
> grouping things together than what we have now.
>> 0003 -- no changes
Numbered 0002 in this next patch set.
> I think it would be better if there were no handler by default, and
> failing to set one leads to an assertion failure when we get to the
> point where one would be called.
Changed to have no default handler, and to use Assert(PointerIsValid(handler)) as you suggest.
> I don't think I understand the point of renaming processQueryResult
> and consumeQueryResult. Isn't that just code churn for its own sake?
I didn't like the names. I had to constantly look back where they were defined to remember which of them
processed/consumedall the results and which only processed/consumed one of them. Part of that problem was that their
namesare both singular. I have restored the names in this next patch set.
> PGresultHandler seems too generic. How about ParallelSlotHandler or
> ParallelSlotResultHandler?
ParallelSlotResultHandler works for me. I'm using that, and renaming
s/TableCommandSlotHandler/TableCommandResultHandler/to be consistent.
> I'm somewhat inclined to propose s/ParallelSlot/ConnectionSlot/g but I
> guess it's better not to get sucked into renaming things.
I admit that I lost a fair amount of time on this project because I thought "scripts_parallel.c" and "parallel_slot"
referredto some kind of threading, but only later looked closely enough to see that this is an event loop, not a
parallelthreading system. I don't think "slot" is terribly informative, and if we rename I don't think it needs to be
partof the name we choose. ConnectionEventLoop would be more intuitive to me than either of
ParallelSlot/ConnectionSlot,but this seems like bikeshedding so I'm going to ignore it for now.
> It's a little strange that we end up with mutators to set the slot's
> handler and handler context when we elsewhere feel free to monkey with
> a slot's connection directly, but it's not a perfect world and I can't
> think of anything I'd like better.
I created those mutators in an earlier version of the patch where the slot had a few more fields to set, and it helped
tohave a single function call set all the fields. I agree it looks less nice now that there are only two fields to
set.
I also made changes to clean up 0003 (formerly numbered 0004)
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company