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
