From: Michael Paquier <michael@paquier.xyz>
To: Julien Rouhaud <rjuju123@gmail.com>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
Cc: Thomas Munro <thomas.munro@gmail.com>
Cc: Daniel Verite <daniel@manitou-mail.org>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Cc: Kevin Grittner <kgrittn@gmail.com>
Subject: Re: Add parallelism and glibc dependent only options to reindexdb
Date: Thu, 11 Jul 2019 13:04:33 +0900
Message-ID: <20190711040433.GG4500@paquier.xyz> (raw)
In-Reply-To: <CAOBaU_Y3Pk-R6M6GSCGBeQtPH_V+h5-ueGaFV+rFOU0oHaGkNg@mail.gmail.com>
References: <CAOBaU_abD08HAxwnx3jGPuBV4POUMmvC_GdMn2qM_+PXsq6f+A@mail.gmail.com>
<20190710141531.GA27615@alvherre.pgsql>
<CAOBaU_Y3Pk-R6M6GSCGBeQtPH_V+h5-ueGaFV+rFOU0oHaGkNg@mail.gmail.com>
On Wed, Jul 10, 2019 at 09:44:14PM +0200, Julien Rouhaud wrote:
> On Wed, Jul 10, 2019 at 4:15 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>> Looking good!
>
> Thanks!
Confirmed. The last set is much easier to go through.
>> I'm not sure about the "Consume" word in ConsumeIdleSlot;
>> maybe "Reserve"? "Obtain"? "Get"?
>
> Yes, Consume is maybe a little bit weird. I wanted to point out the
> make it clear that this function is actually removing a slot from the
> free list, especially since there's a (historical) get_idle_slot(). I
> like Reserve, but Obtain and Get are probably too ambiguous.
The refactoring patch is getting in shape. Now reindex_one_database()
is the only place setting and manipulating the slots. I am wondering
if we should have a wrapper which disconnects all the slots (doing
conn = NULL after the disconnectDatabase() call does not matter).
Get* would be my choice, because we look at the set of parallel slots,
and get an idle one. It would be nice to have more consistency in the
names for the routines, say:
- ParallelSlotInit() instead of SetupParallelSlots (still my
suggestion is not perfect either as that sounds like one single slot,
but we have a set of these).
- ParallelSlotGetIdle() instead of ConsumeIdleSlot(). Still that's
more a wait-then-get behavior.
- ParallelSlotWaitCompletion() instead of WaitForSlotsCompletion()
- ParallelSlotDisconnect, as a wrapper for the calls to
DisconnectDatabase().
>> 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.
>
> I usually prefer to group exported functions together and static ones
> together, as I find it more maintainable in the long term, so upwards
> it'll be.
That's mainly a matter of taste. Depending on the code path in the
tree, sometimes the two approaches from above are used. We have some
other files where the static routines are listed first at the top,
followed by the exported ones at the bottom as it saves from some
declarations of the functions at the top of the file. Keeping the
footprint of the author is not that bad either, and that depends also
on the context. For this one, as the static functions are linked to
the exported ones in a close manner, I would keep each set grouped
though.
+ /*
+ * Database-wide parallel reindex requires special processing. If
+ * multiple jobs were asked, we have to reindex system catalogs first,
+ * as they can't be processed in parallel.
+ */
+ if (process_type == REINDEX_DATABASE)
In patch 0002, a parallel database REINDEX first processes the catalog
relations in a serializable fashion, and then all the other relations
in parallel, which is right Could we have schema-level reindexes also
process things in parallel with all the relations from all the schemas
listed? This would be profitable in particular for callers listing
multiple schemas with an unbalanced number of tables in each, and we'd
need to be careful of the same where pg_catalog is listed. Actually,
your patch breaks if we do a parallel run with pg_catalog and another
schema, no?
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: michael@paquier.xyz, rjuju123@gmail.com, alvherre@2ndquadrant.com, peter.eisentraut@2ndquadrant.com, thomas.munro@gmail.com, daniel@manitou-mail.org, tgl@sss.pgh.pa.us, pgsql-hackers@lists.postgresql.org, kgrittn@gmail.com
Subject: Re: Add parallelism and glibc dependent only options to reindexdb
In-Reply-To: <20190711040433.GG4500@paquier.xyz>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox