On Wed, Jul 17, 2019 at 07:46:10PM +0200, Julien Rouhaud wrote: > On Wed, Jul 17, 2019 at 9:59 AM Michael Paquier <mich...@paquier.xyz> wrote: >> On top of that quick lookup, I have done an in-depth review on 0001 to >> bring it to a committable state, fixing a couple of typos, incorrect >> comments (description of ParallelSlotsGetIdle was for example >> incorrect) on the way. Other things include that connectDatabase >> should have an assertion for a non-NULL connection, > > disconnectDatabase you mean? Fine by me.
Oops, yes. I meant disconnectDatabase() here. The patch does so, not my words. > +/* > + * Disconnect the given connection, canceling any statement if one is active. > + */ > +void > +disconnectDatabase(PGconn *conn) > > Nitpicking, but this comment doesn't follow the style of other > functions' comments (it's also the case for existing comment on > executeQuery at least). connectDatabase, connectMaintenanceDatabase, executeQuery and most of the others follow that style, so I am just going to simplify consumeQueryResult and processQueryResult to keep a consistent style. > While reading the comments you added on ParallelSlotsSetup(), I > wondered if we couldn't also add an Assert(conn) at the beginning? That makes sense as this gets associated to the first slot. There could be an argument for making a set of slots extensible to simplify this interface, but that complicates the logic for the scripts. > Is it ok to call pg_free(slots) and let caller have a pointer pointing > to freed memory? The interface has a Setup call which initializes the whole thing, and Terminate is the logical end point, so having the free logic within the termination looks more consistent to me. We could now have actual Init() and Free() but I am not sure that this justifies the move as this complicates the scripts using it. -- Michael
signature.asc
Description: PGP signature
