On Tue, Feb 2, 2021 at 6:10 PM Mark Dilger <mark.dil...@enterprisedb.com> wrote: > 0001 -- no changes
Committed. > 0002 -- fixing omissions in @pgfeutilsfiles in file > src/tools/msvc/Mkvcbuild.pm Here are a few minor cosmetic issues with this patch: - connect_utils.c lacks a file header comment. - Some or perhaps all of the other file header comments need an update for 2021. - There's bogus hunks in the diff for string_utils.c. 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 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. I don't think I understand the point of renaming processQueryResult and consumeQueryResult. Isn't that just code churn for its own sake? PGresultHandler seems too generic. How about ParallelSlotHandler or ParallelSlotResultHandler? I'm somewhat inclined to propose s/ParallelSlot/ConnectionSlot/g but I guess it's better not to get sucked into renaming things. 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. -- Robert Haas EDB: http://www.enterprisedb.com
