Re: fork() refactoring - Mailing list pgsql-patches

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
Index: src/backend/port/beos/support.c
===================================================================
RCS file: /var/lib/cvs/pgsql/src/backend/port/beos/support.c,v
retrieving revision 1.11
diff -c -r1.11 support.c
*** src/backend/port/beos/support.c    25 Oct 2004 03:23:02 -0000    1.11
--- src/backend/port/beos/support.c    7 Mar 2005 23:36:03 -0000
***************
*** 265,271 ****



! /* The behavior of fork is borken on beos regarding shared memory. In fact
  all shared memory areas are clones in copy on write mode in the new process.

  We need to do a remapping of these areas. Just afer the fork we performe the
--- 265,271 ----



! /* The behavior of fork is broken on beos regarding shared memory. In fact
  all shared memory areas are clones in copy on write mode in the new process.

  We need to do a remapping of these areas. Just afer the fork we performe the
Index: src/backend/postmaster/Makefile
===================================================================
RCS file: /var/lib/cvs/pgsql/src/backend/postmaster/Makefile,v
retrieving revision 1.19
diff -c -r1.19 Makefile
*** src/backend/postmaster/Makefile    5 Aug 2004 23:32:10 -0000    1.19
--- src/backend/postmaster/Makefile    7 Mar 2005 23:36:03 -0000
***************
*** 12,18 ****
  top_builddir = ../../..
  include $(top_builddir)/src/Makefile.global

! OBJS = postmaster.o bgwriter.o pgstat.o pgarch.o syslogger.o

  all: SUBSYS.o

--- 12,18 ----
  top_builddir = ../../..
  include $(top_builddir)/src/Makefile.global

! OBJS = bgwriter.o fork_process.o pgarch.o pgstat.o postmaster.o syslogger.o

  all: SUBSYS.o

Index: src/backend/postmaster/fork_process.c
===================================================================
RCS file: src/backend/postmaster/fork_process.c
diff -N src/backend/postmaster/fork_process.c
*** /dev/null    1 Jan 1970 00:00:00 -0000
--- src/backend/postmaster/fork_process.c    7 Mar 2005 23:36:03 -0000
***************
*** 0 ****
--- 1,80 ----
+ /*
+  * fork_process.c
+  *   A simple wrapper on top of fork(). This does not handle the
+  *   EXEC_BACKEND case; it might be extended to do so, but it would be
+  *   considerably more complex.
+  *
+  * Copyright (c) 1996-2005, PostgreSQL Global Development Group
+  *
+  * IDENTIFICATION
+  *      $PostgreSQL$
+  */
+ #include "postgres.h"
+ #include "postmaster/fork_process.h"
+
+ #include <unistd.h>
+
+ /*
+  * Wrapper for fork(). Return values are the same as those for fork():
+  * -1 if the fork failed, 0 in the child process, and the PID of the
+  * child in the parent process.
+  */
+ pid_t
+ fork_process(void)
+ {
+     pid_t result;
+ #ifdef LINUX_PROFILE
+     struct itimerval prof_itimer;
+ #endif
+
+     /*
+      * Flush stdio channels just before fork, to avoid double-output
+      * problems. Ideally we'd use fflush(NULL) here, but there are still a
+      * few non-ANSI stdio libraries out there (like SunOS 4.1.x) that
+      * coredump if we do. Presently stdout and stderr are the only stdio
+      * output channels used by the postmaster, so fflush'ing them should
+      * be sufficient.
+      */
+     fflush(stdout);
+     fflush(stderr);
+
+ #ifdef LINUX_PROFILE
+     /*
+      * Linux's fork() resets the profiling timer in the child process. If
+      * we want to profile child processes then we need to save and restore
+      * the timer setting.  This is a waste of time if not profiling,
+      * however, so only do it if commanded by specific -DLINUX_PROFILE
+      * switch.
+      */
+     getitimer(ITIMER_PROF, &prof_itimer);
+ #endif
+
+ #ifdef __BEOS__
+     /* Specific beos actions before backend startup */
+     beos_before_backend_startup();
+ #endif
+
+     result = fork();
+     if (result == (pid_t) -1)
+     {
+         /* fork failed */
+ #ifdef __BEOS__
+         /* Specific beos backend startup actions */
+         beos_backend_startup_failed();
+ #endif
+     }
+     else if (result == 0)
+     {
+         /* fork succeeded, in child */
+ #ifdef LINUX_PROFILE
+         setitimer(ITIMER_PROF, &prof_itimer, NULL);
+ #endif
+
+ #ifdef __BEOS__
+         /* Specific beos backend startup actions */
+         beos_backend_startup();
+ #endif
+     }
+
+     return result;
+ }
Index: src/backend/postmaster/pgarch.c
===================================================================
RCS file: /var/lib/cvs/pgsql/src/backend/postmaster/pgarch.c,v
retrieving revision 1.14
diff -c -r1.14 pgarch.c
*** src/backend/postmaster/pgarch.c    31 Dec 2004 22:00:40 -0000    1.14
--- src/backend/postmaster/pgarch.c    7 Mar 2005 23:36:03 -0000
***************
*** 34,39 ****
--- 34,40 ----
  #include "access/xlog_internal.h"
  #include "libpq/pqsignal.h"
  #include "miscadmin.h"
+ #include "postmaster/fork_process.h"
  #include "postmaster/pgarch.h"
  #include "postmaster/postmaster.h"
  #include "storage/fd.h"
***************
*** 141,165 ****
          return 0;
      last_pgarch_start_time = curtime;

-     fflush(stdout);
-     fflush(stderr);
-
- #ifdef __BEOS__
-     /* Specific beos actions before backend startup */
-     beos_before_backend_startup();
- #endif
-
  #ifdef EXEC_BACKEND
      switch ((pgArchPid = pgarch_forkexec()))
  #else
!     switch ((pgArchPid = fork()))
  #endif
      {
          case -1:
- #ifdef __BEOS__
-             /* Specific beos actions */
-             beos_backend_startup_failed();
- #endif
              ereport(LOG,
                      (errmsg("could not fork archiver: %m")));
              return 0;
--- 142,154 ----
          return 0;
      last_pgarch_start_time = curtime;

  #ifdef EXEC_BACKEND
      switch ((pgArchPid = pgarch_forkexec()))
  #else
!     switch ((pgArchPid = fork_process()))
  #endif
      {
          case -1:
              ereport(LOG,
                      (errmsg("could not fork archiver: %m")));
              return 0;
***************
*** 167,176 ****
  #ifndef EXEC_BACKEND
          case 0:
              /* in postmaster child ... */
- #ifdef __BEOS__
-             /* Specific beos actions after backend startup */
-             beos_backend_startup();
- #endif
              /* Close the postmaster's sockets */
              ClosePostmasterPorts(false);

--- 156,161 ----
Index: src/backend/postmaster/postmaster.c
===================================================================
RCS file: /var/lib/cvs/pgsql/src/backend/postmaster/postmaster.c,v
retrieving revision 1.445
diff -c -r1.445 postmaster.c
*** src/backend/postmaster/postmaster.c    22 Feb 2005 04:36:36 -0000    1.445
--- src/backend/postmaster/postmaster.c    7 Mar 2005 23:37:07 -0000
***************
*** 92,97 ****
--- 92,99 ----
  #include <DNSServiceDiscovery/DNSServiceDiscovery.h>
  #endif

+ #include "access/xlog.h"
+ #include "bootstrap/bootstrap.h"
  #include "catalog/pg_control.h"
  #include "catalog/pg_database.h"
  #include "commands/async.h"
***************
*** 103,125 ****
  #include "libpq/pqsignal.h"
  #include "miscadmin.h"
  #include "nodes/nodes.h"
! #include "postmaster/postmaster.h"
  #include "postmaster/pgarch.h"
  #include "postmaster/syslogger.h"
  #include "storage/fd.h"
  #include "storage/ipc.h"
  #include "storage/pg_shmem.h"
  #include "storage/pmsignal.h"
  #include "storage/proc.h"
- #include "storage/bufmgr.h"
- #include "access/xlog.h"
  #include "tcop/tcopprot.h"
  #include "utils/builtins.h"
  #include "utils/guc.h"
  #include "utils/memutils.h"
  #include "utils/ps_status.h"
- #include "bootstrap/bootstrap.h"
- #include "pgstat.h"

  #ifdef EXEC_BACKEND
  #include "storage/spin.h"
--- 105,126 ----
  #include "libpq/pqsignal.h"
  #include "miscadmin.h"
  #include "nodes/nodes.h"
! #include "pgstat.h"
! #include "postmaster/fork_process.h"
  #include "postmaster/pgarch.h"
+ #include "postmaster/postmaster.h"
  #include "postmaster/syslogger.h"
+ #include "storage/bufmgr.h"
  #include "storage/fd.h"
  #include "storage/ipc.h"
  #include "storage/pg_shmem.h"
  #include "storage/pmsignal.h"
  #include "storage/proc.h"
  #include "tcop/tcopprot.h"
  #include "utils/builtins.h"
  #include "utils/guc.h"
  #include "utils/memutils.h"
  #include "utils/ps_status.h"

  #ifdef EXEC_BACKEND
  #include "storage/spin.h"
***************
*** 1008,1023 ****
      int            i;
      pid_t        pid;

! #ifdef LINUX_PROFILE
!     struct itimerval prof_itimer;
! #endif
!
! #ifdef LINUX_PROFILE
!     /* see comments in BackendStartup */
!     getitimer(ITIMER_PROF, &prof_itimer);
! #endif
!
!     pid = fork();
      if (pid == (pid_t) -1)
      {
          write_stderr("%s: could not fork background process: %s\n",
--- 1009,1015 ----
      int            i;
      pid_t        pid;

!     pid = fork_process();
      if (pid == (pid_t) -1)
      {
          write_stderr("%s: could not fork background process: %s\n",
***************
*** 1030,1039 ****
          _exit(0);
      }

- #ifdef LINUX_PROFILE
-     setitimer(ITIMER_PROF, &prof_itimer, NULL);
- #endif
-
      MyProcPid = PostmasterPid = getpid();        /* reset PID vars to child */

  /* GH: If there's no setsid(), we hopefully don't need silent mode.
--- 1022,1027 ----
***************
*** 2382,2391 ****
      Backend    *bn;                /* for backend cleanup */
      pid_t        pid;

- #ifdef LINUX_PROFILE
-     struct itimerval prof_itimer;
- #endif
-
      /*
       * Compute the cancel key that will be assigned to this backend. The
       * backend will have its own copy in the forked-off process' value of
--- 2370,2375 ----
***************
*** 2409,2462 ****
      /* Pass down canAcceptConnections state (kluge for EXEC_BACKEND case) */
      port->canAcceptConnections = canAcceptConnections();

-     /*
-      * Flush stdio channels just before fork, to avoid double-output
-      * problems. Ideally we'd use fflush(NULL) here, but there are still a
-      * few non-ANSI stdio libraries out there (like SunOS 4.1.x) that
-      * coredump if we do. Presently stdout and stderr are the only stdio
-      * output channels used by the postmaster, so fflush'ing them should
-      * be sufficient.
-      */
-     fflush(stdout);
-     fflush(stderr);
-
  #ifdef EXEC_BACKEND
-
      pid = backend_forkexec(port);
-
  #else                            /* !EXEC_BACKEND */
!
! #ifdef LINUX_PROFILE
!
!     /*
!      * Linux's fork() resets the profiling timer in the child process. If
!      * we want to profile child processes then we need to save and restore
!      * the timer setting.  This is a waste of time if not profiling,
!      * however, so only do it if commanded by specific -DLINUX_PROFILE
!      * switch.
!      */
!     getitimer(ITIMER_PROF, &prof_itimer);
! #endif
!
! #ifdef __BEOS__
!     /* Specific beos actions before backend startup */
!     beos_before_backend_startup();
! #endif
!
!     pid = fork();
!
      if (pid == 0)                /* child */
      {
- #ifdef LINUX_PROFILE
-         setitimer(ITIMER_PROF, &prof_itimer, NULL);
- #endif
-
- #ifdef __BEOS__
-         /* Specific beos backend startup actions */
-         beos_backend_startup();
- #endif
          free(bn);
-
          proc_exit(BackendRun(port));
      }
  #endif   /* EXEC_BACKEND */
--- 2393,2405 ----
      /* Pass down canAcceptConnections state (kluge for EXEC_BACKEND case) */
      port->canAcceptConnections = canAcceptConnections();

  #ifdef EXEC_BACKEND
      pid = backend_forkexec(port);
  #else                            /* !EXEC_BACKEND */
!     pid = fork_process();
      if (pid == 0)                /* child */
      {
          free(bn);
          proc_exit(BackendRun(port));
      }
  #endif   /* EXEC_BACKEND */
***************
*** 2466,2475 ****
          /* in parent, fork failed */
          int            save_errno = errno;

- #ifdef __BEOS__
-         /* Specific beos backend startup actions */
-         beos_backend_startup_failed();
- #endif
          free(bn);
          errno = save_errno;
          ereport(LOG,
--- 2409,2414 ----
***************
*** 2945,2951 ****
      argv[2] = tmpfilename;

      /* Fire off execv in child */
!     if ((pid = fork()) == 0)
      {
          if (execv(postgres_exec_path, argv) < 0)
          {
--- 2884,2890 ----
      argv[2] = tmpfilename;

      /* Fire off execv in child */
!     if ((pid = fork_process()) == 0)
      {
          if (execv(postgres_exec_path, argv) < 0)
          {
***************
*** 3465,3474 ****
      int            ac = 0;
      char        xlbuf[32];

- #ifdef LINUX_PROFILE
-     struct itimerval prof_itimer;
- #endif
-
      /*
       * Set up command-line arguments for subprocess
       */
--- 3404,3409 ----
***************
*** 3488,3528 ****
      av[ac] = NULL;
      Assert(ac < lengthof(av));

-     /*
-      * Flush stdio channels (see comments in BackendStartup)
-      */
-     fflush(stdout);
-     fflush(stderr);
-
  #ifdef EXEC_BACKEND
-
      pid = postmaster_forkexec(ac, av);
-
  #else                            /* !EXEC_BACKEND */
!
! #ifdef LINUX_PROFILE
!     /* see comments in BackendStartup */
!     getitimer(ITIMER_PROF, &prof_itimer);
! #endif
!
! #ifdef __BEOS__
!     /* Specific beos actions before backend startup */
!     beos_before_backend_startup();
! #endif
!
!     pid = fork();

      if (pid == 0)                /* child */
      {
- #ifdef LINUX_PROFILE
-         setitimer(ITIMER_PROF, &prof_itimer, NULL);
- #endif
-
- #ifdef __BEOS__
-         /* Specific beos actions after backend startup */
-         beos_backend_startup();
- #endif
-
          IsUnderPostmaster = true;        /* we are a postmaster subprocess
                                           * now */

--- 3423,3435 ----
      av[ac] = NULL;
      Assert(ac < lengthof(av));

  #ifdef EXEC_BACKEND
      pid = postmaster_forkexec(ac, av);
  #else                            /* !EXEC_BACKEND */
!     pid = fork_process();

      if (pid == 0)                /* child */
      {
          IsUnderPostmaster = true;        /* we are a postmaster subprocess
                                           * now */

***************
*** 3546,3556 ****
      {
          /* in parent, fork failed */
          int            save_errno = errno;
-
- #ifdef __BEOS__
-         /* Specific beos actions before backend startup */
-         beos_backend_startup_failed();
- #endif
          errno = save_errno;
          switch (xlop)
          {
--- 3453,3458 ----
Index: src/backend/postmaster/syslogger.c
===================================================================
RCS file: /var/lib/cvs/pgsql/src/backend/postmaster/syslogger.c,v
retrieving revision 1.12
diff -c -r1.12 syslogger.c
*** src/backend/postmaster/syslogger.c    1 Jan 2005 20:44:16 -0000    1.12
--- src/backend/postmaster/syslogger.c    7 Mar 2005 23:36:03 -0000
***************
*** 33,38 ****
--- 33,39 ----

  #include "libpq/pqsignal.h"
  #include "miscadmin.h"
+ #include "postmaster/fork_process.h"
  #include "postmaster/postmaster.h"
  #include "postmaster/syslogger.h"
  #include "pgtime.h"
***************
*** 451,478 ****

      pfree(filename);

-     /*
-      * Now we can fork off the syslogger subprocess.
-      */
-     fflush(stdout);
-     fflush(stderr);
-
- #ifdef __BEOS__
-     /* Specific beos actions before backend startup */
-     beos_before_backend_startup();
- #endif
-
  #ifdef EXEC_BACKEND
      switch ((sysloggerPid = syslogger_forkexec()))
  #else
!     switch ((sysloggerPid = fork()))
  #endif
      {
          case -1:
- #ifdef __BEOS__
-             /* Specific beos actions */
-             beos_backend_startup_failed();
- #endif
              ereport(LOG,
                      (errmsg("could not fork system logger: %m")));
              return 0;
--- 452,464 ----

      pfree(filename);

  #ifdef EXEC_BACKEND
      switch ((sysloggerPid = syslogger_forkexec()))
  #else
!     switch ((sysloggerPid = fork_process()))
  #endif
      {
          case -1:
              ereport(LOG,
                      (errmsg("could not fork system logger: %m")));
              return 0;
***************
*** 480,489 ****
  #ifndef EXEC_BACKEND
          case 0:
              /* in postmaster child ... */
- #ifdef __BEOS__
-             /* Specific beos actions after backend startup */
-             beos_backend_startup();
- #endif
              /* Close the postmaster's sockets */
              ClosePostmasterPorts(true);

--- 466,471 ----
Index: src/include/postmaster/fork_process.h
===================================================================
RCS file: src/include/postmaster/fork_process.h
diff -N src/include/postmaster/fork_process.h
*** /dev/null    1 Jan 1970 00:00:00 -0000
--- src/include/postmaster/fork_process.h    7 Mar 2005 23:36:03 -0000
***************
*** 0 ****
--- 1,8 ----
+ #ifndef FORK_PROCESS_H
+ #define FORK_PROCESS_H
+
+ #include "postgres.h"
+
+ extern pid_t fork_process(void);
+
+ #endif /* ! FORK_PROCESS_H */