On Thu, Jul 11, 2019 at 3:34 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Thu, Jul 11, 2019 at 11:48:20AM +0200, Julien Rouhaud wrote:
> > On Thu, Jul 11, 2019 at 6:04 AM Michael Paquier <michael@paquier.xyz> wrote:
> >> Get* would be my choice, because we look at the set of parallel slots,
> >> and get an idle one.
> >
> > That's what the former GetIdleSlot (that I renamed to get_idle_slot as
> > it's not static) is doing. ConsumeIdleSlot() actually get an idle
> > slot and mark it as being used. That's probably some leakage of
> > internal implementation, but having a GetIdleParallelSlot (or
> > ParallelSlotGetIdle) *and* a get_idle_slot sounds like a bad idea, and
> > I don't have a better idea on how to rename get_idle_slot. If you
> > really prefer Get* and you're fine with a static get_idle_slot, that's
> > fine by me.
>
> Do we actually need get_idle_slot? ConsumeIdleSlot is its only
> caller.
I think t hat it makes the code quite cleaner to have the selection
outside ConsumeIdleSlot.
> And while scanning the code...
>
> + * getQuerySucess
> Typo here.
Argh, I thought I caught all of them, thanks!
> - * Pump the conn till it's dry of results; return false if any are errors.
> This comment could be improved on the way, like "Go through all the
> connections and make sure to consume any remaining results. If any
> error is found, false is returned after processing all the parallel
> slots."
You're talking about getQuerySuccess right? That was actually the
original comment of a function I renamed. +1 to improve it, but this
function is in common.c and doesn't deal with parallel slot at all, so
I'll just drop the slang parts.
