Re: new heapcheck contrib module

> 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/consumed all the results and which only processed/consumed one of them.  Part of that problem was that their names are 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" referred to some kind of threading, but only later looked closely enough to see that this is an event loop, not a parallel threading system.  I don't think "slot" is terribly informative, and if we rename I don't think it needs to be part of 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 to have 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)