Hi Alvaro,
Thanks a lot for the review
On Wed, Jul 10, 2019 at 4:15 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>
> On 2019-Jul-09, Julien Rouhaud wrote:
>
> > I finished to do a better refactoring, and ended up with this API in
> > scripts_parallel:
>
> Looking good!
Thanks!
> I'm not sure about the "Consume" word in ConsumeIdleSlot;
> maybe "Reserve"? "Obtain"? "Get"?
Yes, Consume is maybe a little bit weird. I wanted to point out the
make it clear that this function is actually removing a slot from the
free list, especially since there's a (historical) get_idle_slot(). I
like Reserve, but Obtain and Get are probably too ambiguous.
> Code commentary: I think the comment that sits atop the function should
> describe what the function does without getting too much in how it does
> it. For example in ConsumeIdleSlot you have "If there are multiples
> slots, here we wait for one connection to become available if none
> already is, returning NULL if an error occured. Otherwise, we simply
> use the only slot we have, which we know to be free." which seems like
> it should be in another comment *inside* the function; make the external
> one something like "Reserve and return a connection that is currently
> idle, waiting until one becomes idle if none is". Maybe you can put the
> part I first quoted as a second paragraph in the comment at top of
> function and keeping the second part I quoted as first paragraph; we
> seem to use that style too.
Good point, I'll fix as you say.
> Placement: I think it's good if related functions stay together, or
> there is some other rationale for placement within the file. I have two
> favorite approaches: one is to put all externally callable functions at
> top of file, followed by all the static helpers in the lower half of the
> file. The other is to put each externally accessible immediately
> followed by its specific static helpers. If you choose one of those,
> that means that SetupParallelSlots should either move upwards, or move
> downwards. The current ordering seems a dartboard kind of thing where
> the thrower is not Green Arrow.
:) I tried to put everything in alphabetic order as it was previously
being done, but I apparently failed again at sorting more than 3
characters.
I usually prefer to group exported functions together and static ones
together, as I find it more maintainable in the long term, so upwards
it'll be.
