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! I'm not sure about the "Consume" word in ConsumeIdleSlot;
maybe "Reserve"? "Obtain"? "Get"?
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.
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.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
