Redesigning parallel dump/restore's wait-for-workers logic

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: pgsql-hackers(at)postgreSQL(dot)org
Subject: Redesigning parallel dump/restore's wait-for-workers logic
Date: 2016-05-29 17:54:03
Message-ID: 1188.1464544443@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

Attachment Content-Type Size
parallel-dump-waiting.patch text/x-diff 34.1 KB

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Andreas Seltenreich 2016-05-29 20:22:15 Re: [sqlsmith] PANIC: failed to add BRIN tuple
Previous Message Emre Hasegeli 2016-05-29 17:28:59 regexp_match() returning text