Hi Alvaro, Thanks a lot for the review
On Wed, Jul 10, 2019 at 4:15 PM Alvaro Herrera <alvhe...@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.
