fork() refactoring

Neil Conway <neilc@samurai.com> writes:

This patch moves all the common code that is usually invoked before
doing a fork() into a single function, fork_process(). It is not aware
of the EXEC_BACKEND machinery, so it should be used as fork() currently
is -- inside an #ifndef EXEC_BACKEND block, if appropriate.

I'm worried about whether this doesn't break the EXEC_BACKEND case.
Most of the code you've moved out isn't applicable to Windows, but
the fflushes probably are --- and they are certainly applicable when
testing EXEC_BACKEND mode on a Unix machine, which is a case you may
*not* break because it will render Windows completely unsupportable.

Barring any objections, I'll apply this to HEAD on Monday.

Please do not apply without some further portability testing.

I think it would be better to continue with your original thought of
passing a token into this code so that the EXEC_BACKEND case could be
handled too (the token would tell it which forkexec function to call).
That would probably mean that the function has to stay within
postmaster.c, but as long as it consolidates the N cases of fork
decoration into one, we're still ahead of the game.

regards, tom lane

Tom Lane wrote:

I'm worried about whether this doesn't break the EXEC_BACKEND case.
Most of the code you've moved out isn't applicable to Windows, but
the fflushes probably are

Right, which is why the patch adds fflushes to the Unix implementation
of internal_forkexec(). On reflection, it is probably more
straightforward to just invoke fork_process() from the Unix version of
internal_forkexec() -- attached is a revised patch that does this.

Please do not apply without some further portability testing.

I've checked EXEC_BACKEND on Unix, and it seems to work. I don't have
access to a BeOS box, so I can't test that (and I wouldn't be surprised
if the port was pretty bitrotted already). Is there anything else you
want me to test before committing?

I think it would be better to continue with your original thought of
passing a token into this code so that the EXEC_BACKEND case could be
handled too (the token would tell it which forkexec function to call).

The problem is that this was getting pretty complex; a lot of the
forkexec implementations want to pass data to the child process that is
private to the forkexec call site, for example. That means we still need
an #ifdef EXEC_BACKEND -- i.e. it's not such a win over the simpler
fork_process(). Since Magnus said he's thinking of refactoring this
anyway, I'm happy to leave it to him.

-Neil

Neil Conway wrote:

Right, which is why the patch adds fflushes to the Unix implementation
of internal_forkexec(). On reflection, it is probably more
straightforward to just invoke fork_process() from the Unix version of
internal_forkexec() -- attached is a revised patch that does this.

I'll apply this patch to HEAD later today, barring any objections.

-Neil

Neil Conway wrote:

I'll apply this patch to HEAD later today, barring any objections.

Applied.

-Neil