Re: patch for parallel pg_dump

On Thu, Feb 23, 2012 at 11:37 PM, Joachim Wieland <joe(at)mcknight(dot)de> wrote:
> On Thu, Feb 16, 2012 at 1:29 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> Can you provide an updated patch?
>
> Robert, updated patch is attached.

Well, I was hoping someone else would do some work on this, but here
we are. Some more comments from me:

+ /*
+ * If the version is lower and we don't have
synchronized snapshots
+ * yet, we will error out earlier already. So
either we have the
+ * feature or the user has given the explicit
command not to use it.
+ * Note: If we have it, we always use it, you
cannot switch it off
+ * then.
+ */

I don't agree with this design decision. I think that
--no-synchronized-snapshots should be available even on server
versions that support it.

+ if (archiveFormat != archDirectory && numWorkers > 1) {

Style.

- const char *owner, bool withOids,
+ const char *owner,
+ unsigned long int relpages, bool withOids,

The new argument to ArchiveEntry() is unused. Removing it would
declutter things a good bit.

+#include "../../backend/port/pipe.c"

This seems really ugly. I suggest that we commit a separate patch to
move src/backend/port/pipe.c to src/port and refactor the interface so
that it has a signature something like this:

int win32_setup_pipe(int handles[2], char **error_string, int *error_code);

The backend can have a wrapper function around this that calls ereport
using the error_string and error_code, and any front-end code that
wants to use this can do so directly.

+/*
+ * The parallel error handler is called for any die_horribly() in a
child or master process.
+ * It then takes control over shutting down the rest of the gang.
+ */

I think this needs to be revised to take control in exit_nicely(),
maybe by using on_exit_nicely(). Trapping die_horribly() won't catch
everything.

+ if (msgsize == 0 && !do_wait) {
+ setnonblocking(fd);
+ }

Style.

+ if (msg[msgsize] == '\0') {
+ return msg;
+ }

Style.

I find myself somewhat uncomfortable with the extent to which this is
relying on being able to set blocking and nonblocking flags on and off
repeatedly. Maybe there's no real problem with it except for being
inefficient, but the way I'd expect this to readMessageFromPipe() to
be written is: Always leave the pipe in blocking mode. If you want a
non-blocking read, then use select() first to check whether it's ready
for read; if not, just do the read directly. Actually, looking
further, it seems that you already have such logic in
getMessageFromWorker(), so I'm unclear why do_wait needs to be passed
down to readMessageFromPipe() at all.

+ * Hence this function returns an (unsigned) int.
+ */
+static int

It doesn't look unsigned?

+extern const char *fmtQualifiedId(struct Archive *fout,
+
const char *schema, const char *id);

I don't think we want to expose struct Archive to dumputils.h. Can we
find a different place to put this?

+enum escrow_action { GET, SET };
+static void
+parallel_error_handler_escrow_data(enum escrow_action act,
ParallelState *pstate)
+{
+ static ParallelState *s_pstate = NULL;
+
+ if (act == SET)
+ s_pstate = pstate;
+ else
+ *pstate = *s_pstate;
+}

This seems like a mighty complicated way to implement a global variable.

+#ifdef HAVE_SETSID
+ /*
+ * If we can, we try to make each process the
leader of its own
+ * process group. The reason is that if you
hit Ctrl-C and they are
+ * all in the same process group, any
termination sequence is
+ * possible, because every process will
receive the signal. What
+ * often happens is that a worker receives the
signal, terminates
+ * and the master detects that one of the
workers had a problem,
+ * even before acting on its own signal.
That's still okay because
+ * everyone still terminates but it looks a bit weird.
+ *
+ * With setsid() however, a Ctrl-C is only
sent to the master and
+ * he can then cascade it to the worker processes.
+ */
+ setsid();
+#endif

This doesn't seem like a very good idea, because if the master fails
to propagate the ^C to all the workers for any reason, then the user
will have to hunt them down manually and kill them. Or imagine that
someone hits ^Z, for example. I think the right thing to do is to
have the workers catch the signal and handle it by terminating,
passing along a notification to the master that they were terminated
by user action; then the master can DTRT.

+typedef struct {

Style.

There's probably more, but the non-PostgreSQL part of my life is calling me.

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