Redesigning parallel dump/restore's wait-for-workers logic
One of the things I do not like about the current coding of parallel
pg_dump/pg_restore is its baroque logic for handling worker completion
reports, specifically the ListenToWorkers/ReapWorkerStatus APIs.
That code is messy and hard to use --- if the existing logic in
restore_toc_entries_parallel doesn't make your head hurt, you're a better
man than I am. And we've got two other similar loops using those
functions, which cry out to be merged but can't be because the other two
have hard-wired ideas about what the cleanup action is.
Hence, I propose the attached redesign. This is based on the idea of
having DispatchJobForTocEntry register a callback function that will take
care of state cleanup, doing whatever had been done by the caller of
ReapWorkerStatus in the old design. (This callback is essentially just
the old mark_work_done function in the restore case, and a trivial test
for worker failure in the dump case.) Then we can have ListenToWorkers
call the callback immediately on receipt of a status message, and return
the worker to WRKR_IDLE state; so the WRKR_FINISHED state goes away.
And it becomes easy to design a unified wait-for-worker-messages loop:
in the attached, WaitForWorkers replaces EnsureIdleWorker and
EnsureWorkersFinished as well as the mess in restore_toc_entries_parallel.
Also, we no longer need the fragile API spec that the caller of
DispatchJobForTocEntry is responsible for ensuring there's an idle worker.
In passing, I got rid of the ParallelArgs struct, which was a net negative
in terms of notational verboseness, and didn't seem to be providing any
noticeable amount of abstraction either.
BTW, I also tried to make ParallelState an opaque struct known only
within parallel.c. I failed at that because there are two loops in
get_next_work_item that want to look at all the actively-being-worked-on
TocEntrys. A possible solution to that is to separate the TocEntry
fields into their own array, so that ParallelState looks like
typedef struct ParallelState
{
int numWorkers; /* allowed number of workers */
/* these arrays have numWorkers entries, one per worker: */
TocEntry **te; /* item being worked on, or NULL */
ParallelSlot *parallelSlot; /* private info about each worker */
} ParallelState;
where ParallelSlot could be opaque outside parallel.c. I'm not sure
if this is worth the trouble though.
Comments?
regards, tom lane
Attachments:
parallel-dump-waiting.patchtext/x-diff; charset=us-ascii; name=parallel-dump-waiting.patchDownload+419-414
I wrote:
One of the things I do not like about the current coding of parallel
pg_dump/pg_restore is its baroque logic for handling worker completion
reports, specifically the ListenToWorkers/ReapWorkerStatus APIs.
Here's a version of this patch rebased over e652273e073566b6.
Since this is only refactoring, and doesn't (I think) fix any bugs,
I'm setting it aside till the next commitfest.
regards, tom lane
Attachments:
parallel-dump-waiting-2.patchtext/x-diff; charset=us-ascii; name=parallel-dump-waiting-2.patchDownload+433-416
This patch applies with a few minor offsets, compiles without
warning, and passes all regression tests including `make
check-world` with TAP tests enabled. This and the related patch
dealing with the parallel API definitely make things cleaner and
easier to follow.
I find it disappointing that ACLs continue to be held back as a
fourth step outside the framework of the "normal" ordering. That
is an ugly hack, which makes it impossible to, for example, create
a fifth step to create indexes on materialized views and refresh
them in anything resembling a clean fashion. Would it make sense
to deal with the ACL ordering hack in one of these patches, or
should that be left for later?
--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Kevin Grittner <kgrittn@gmail.com> writes:
This patch applies with a few minor offsets, compiles without
warning, and passes all regression tests including `make
check-world` with TAP tests enabled. This and the related patch
dealing with the parallel API definitely make things cleaner and
easier to follow.
OK, thanks for reviewing!
I find it disappointing that ACLs continue to be held back as a
fourth step outside the framework of the "normal" ordering. That
is an ugly hack, which makes it impossible to, for example, create
a fifth step to create indexes on materialized views and refresh
them in anything resembling a clean fashion. Would it make sense
to deal with the ACL ordering hack in one of these patches, or
should that be left for later?
Seems like material for some other patch. It would be greatly more
invasive than what I have here, I fear, and probably require some
fundamental design work.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers