Re: Add parallelism and glibc dependent only options to reindexdb

On Wed, Jul 17, 2019 at 9:59 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Tue, Jul 16, 2019 at 02:03:16PM +0200, Julien Rouhaud wrote:
> > After more thinking about schema and multiple jobs, I think that
> > erroring out is quite user unfriendly, as it's entirely ok to ask
> > for
> > multiple indexes and multiple object that do support parallelism in
> > a
> > single call.  So I think it's better to remove the error, ignore the
> > given --jobs options for indexes and document this behavior.
>
> No objections to that.  I still need to study a bit more 0002 though
> to come to a clear conclusion.
>
> Actually, from patch 0002:
> +       free_slot = ParallelSlotsGetIdle(slots, concurrentCons, progname);
> +       if (!free_slot)
> +       {
> +           failed = true;
> +           goto finish;
> +       }
> +
> +       run_reindex_command(conn, process_type, objname, progname, echo,
> +                           verbose, concurrently, true);
> The same connection gets reused, shouldn't the connection come from
> the free slot?

Ouch indeed.

> 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.

> calling pg_free()
> on the slots terminate is more consistent as pg_malloc is used first.
> A comment at the top of processQueryResult still referred to
> vacuuming of a missing relation.  Most of the patch was in a clean
> state, with a clear interface for parallel slots, the place of the new
> routines also makes sense, so I did not have much to do :)

Thanks :)

> Another thing I have noticed is that we don't really need to pass down
> progname across all those layers as we finish by using pg_log_error()
> when processing results, so more simplifications can be done.  Let's
> handle all that in the same patch as we are messing with the area.
> connectDatabase() and connectMaintenanceDatabase() still need it
> though as this is used in the connection string, so
> ParallelSlotsSetup() also needs it.  This part is not really your
> fault but as I am looking at it, it does not hurt to clean up what we
> can.  Attached is an updated version of 0001 that I am comfortable
> with.  I'd like to commit that with the cleanups included and then
> let's move to the real deal with 0002.

Good catch, I totally missed this progname change.  I read the patch
you attached, I have a few comments:

+/*
+ * 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).


While reading the comments you added on ParallelSlotsSetup(), I
wondered if we couldn't also add an Assert(conn) at the beginning?

+void
+ParallelSlotsTerminate(ParallelSlot *slots, int numslots)
+{
+ int i;
+
+ for (i = 0; i < numslots; i++)
+ {
+ PGconn    *conn = slots[i].connection;
+
+ if (conn == NULL)
+ continue;
+
+ disconnectDatabase(conn);
+ }
+
+ pg_free(slots);
+}

Is it ok to call pg_free(slots) and let caller have a pointer pointing
to freed memory?