Re: patch for parallel pg_dump

On Sun, Mar 25, 2012 at 10:50 PM, Joachim Wieland <joe(at)mcknight(dot)de> wrote:
> On Fri, Mar 23, 2012 at 11:11 AM, Alvaro Herrera
> <alvherre(at)commandprompt(dot)com> wrote:
>> Are you going to provide a rebased version?
>
> Rebased version attached, this patch also includes Robert's earlier suggestions.

I keep hoping someone who knows Windows is going to take a look at
this, but so far no luck. It could also really use some attention
from someone who has an actual really big database handy, to see how
successful it is in reducing the dump time. Without those things, I
can't see this getting committed. But in the meantime, a few fairly
minor comments based on reading the code.

I'm wondering if we really need this much complexity around shutting
down workers. I'm not sure I understand why we need both a "hard" and
a "soft" method of shutting them down. At least on non-Windows
systems, it seems like it would be entirely sufficient to just send a
SIGTERM when you want them to die. They don't even need to catch it;
they can just die. You could also set things up so that if the
connection to the parent process is closed, the worker exits. Then,
during normal shutdown, you don't need to kill them at all. The
master can simply exit, and the child processes will follow suit. The
checkAborting stuff all goes away.

The existing coding of on_exit_nicely is intention, and copied from
similar logic for on_shmem_exit in the backend. Is there really a
compelling reason to reverse the firing order of exit hooks?

On my system:

parallel.c: In function ‘WaitForTerminatingWorkers’:
parallel.c:275: warning: ‘slot’ may be used uninitialized in this function
make: *** [parallel.o] Error 1

Which actually looks like a semi-legitimate gripe.

+ if (numWorkers > MAXIMUM_WAIT_OBJECTS)
+ {
+ fprintf(stderr, _("%s: invalid number of parallel
jobs\n"), progname);
+ exit(1);
+ }

I think this error message could be more clear. How about "maximum
number of parallel jobs is %d"?

+void _SetupWorker(Archive *AHX, RestoreOptions *ropt) {}

Thankfully, this bit in pg_dumpall.c appears to be superfluous. I
hope this is just a holdover from an earlier version that we can lose.

- const char *modulename,
const char *fmt,...)
+ const char *modulename,
const char *fmt,...)

Useless hunk.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company