On Wed, Jul 17, 2019 at 07:46:10PM +0200, Julien Rouhaud wrote:
> On Wed, Jul 17, 2019 at 9:59 AM Michael Paquier <michael@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
