Re: BUG #15804: Assertion failure when using logging_collector with EXEC_BACKEND

I wrote:
> This line of thought suggests that trying to fix things so that
> we can launch child processes before creating shared memory
> is the wrong thing, because it seriously risks creating problems
> in the leftover-child-processes scenario.

> This means that the change that 57431a911 wanted to make is only
> going to be safe if we're willing to re-order things so that the
> startup sequence is

>     * create datadir lock file
>     * create shmem
>     * launch syslogger
>     * create sockets

In other words, the right way to think about this is less "move
syslogger launch to earlier" and more "move port opening to later".

I did some cursory testing of that idea with the attached patch,
which simply relocates the port opening logic to below where
syslogger start is (though "git diff" insists on presenting it
differently :-().  I also moved and recommented the emission
of the "starting ..." log entry.  It works under EXEC_BACKEND,
but I'm not fool enough to believe that that proves it works
under Windows :-(.

One issue with this is that we can't be sure we have sole control
of the postmaster port number at the time we create shmem.
Hence, to avoid undesirable conflicts of shmem, we should change
things to base the shmem key on the datadir's ID not the port
number, as was already speculated about in
https://postgr.es/m/16908.1557521200@sss.pgh.pa.us

Also, this will change the order in which entries get made into
postmaster.pid.  I think that's OK, but we'll need to take a
close look at pg_ctl to be sure it isn't making any invalid
assumptions.

Another point is that we want to be sure this doesn't change
the order in which lockfiles are released at shutdown.  That
seems OK (I confirmed by strace'ing that the postmaster's
final syscalls are still done in the same order) but it could
use some additional eyeballs on it.

There may be some other reorderings that would be a good idea.
In particular I'm thinking that the CreateOptsFile call should
be pushed down, so that it doesn't get written until we know
that the port number is OK.

            regards, tom lane

diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index f2c99e6..5ec63aa 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -993,7 +993,140 @@ PostmasterMain(int argc, char *argv[])
      */
     InitializeMaxBackends();

-    /* Report server startup in log */
+    /*
+     * Set up shared memory and semaphores.
+     */
+    reset_shared(PostPortNumber);
+
+    /*
+     * Estimate number of openable files.  This must happen after setting up
+     * semaphores, because on some platforms semaphores count as open files.
+     */
+    set_max_safe_fds();
+
+    /*
+     * Set reference point for stack-depth checking.
+     */
+    set_stack_base();
+
+    /*
+     * Initialize pipe (or process handle on Windows) that allows children to
+     * wake up from sleep on postmaster death.
+     */
+    InitPostmasterDeathWatchHandle();
+
+#ifdef WIN32
+
+    /*
+     * Initialize I/O completion port used to deliver list of dead children.
+     */
+    win32ChildQueue = CreateIoCompletionPort(INVALID_HANDLE_VALUE, NULL, 0, 1);
+    if (win32ChildQueue == NULL)
+        ereport(FATAL,
+                (errmsg("could not create I/O completion port for child queue")));
+#endif
+
+    /*
+     * Record postmaster options.  We delay this till now to avoid recording
+     * bogus options (eg, NBuffers too high for available memory).
+     */
+    if (!CreateOptsFile(argc, argv, my_exec_path))
+        ExitPostmaster(1);
+
+#ifdef EXEC_BACKEND
+    /* Write out nondefault GUC settings for child processes to use */
+    write_nondefault_variables(PGC_POSTMASTER);
+#endif
+
+    /*
+     * Write the external PID file if requested
+     */
+    if (external_pid_file)
+    {
+        FILE       *fpidfile = fopen(external_pid_file, "w");
+
+        if (fpidfile)
+        {
+            fprintf(fpidfile, "%d\n", MyProcPid);
+            fclose(fpidfile);
+
+            /* Make PID file world readable */
+            if (chmod(external_pid_file, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH) != 0)
+                write_stderr("%s: could not change permissions of external PID file \"%s\": %s\n",
+                             progname, external_pid_file, strerror(errno));
+        }
+        else
+            write_stderr("%s: could not write external PID file \"%s\": %s\n",
+                         progname, external_pid_file, strerror(errno));
+
+        on_proc_exit(unlink_external_pid_file, 0);
+    }
+
+    /*
+     * Remove old temporary files.  At this point there can be no other
+     * Postgres processes running in this directory, so this should be safe.
+     */
+    RemovePgTempFiles();
+
+    /*
+     * Forcibly remove the files signaling a standby promotion request.
+     * Otherwise, the existence of those files triggers a promotion too early,
+     * whether a user wants that or not.
+     *
+     * This removal of files is usually unnecessary because they can exist
+     * only during a few moments during a standby promotion. However there is
+     * a race condition: if pg_ctl promote is executed and creates the files
+     * during a promotion, the files can stay around even after the server is
+     * brought up to new master. Then, if new standby starts by using the
+     * backup taken from that master, the files can exist at the server
+     * startup and should be removed in order to avoid an unexpected
+     * promotion.
+     *
+     * Note that promotion signal files need to be removed before the startup
+     * process is invoked. Because, after that, they can be used by
+     * postmaster's SIGUSR1 signal handler.
+     */
+    RemovePromoteSignalFiles();
+
+    /* Do the same for logrotate signal file */
+    RemoveLogrotateSignalFiles();
+
+    /* Remove any outdated file holding the current log filenames. */
+    if (unlink(LOG_METAINFO_DATAFILE) < 0 && errno != ENOENT)
+        ereport(LOG,
+                (errcode_for_file_access(),
+                 errmsg("could not remove file \"%s\": %m",
+                        LOG_METAINFO_DATAFILE)));
+
+    /*
+     * If enabled, start up syslogger collection subprocess
+     */
+    SysLoggerPID = SysLogger_Start();
+
+    /*
+     * Reset whereToSendOutput from DestDebug (its starting state) to
+     * DestNone. This stops ereport from sending log messages to stderr unless
+     * Log_destination permits.  We don't do this until the postmaster is
+     * fully launched, since startup failures may as well be reported to
+     * stderr.
+     *
+     * If we are in fact disabling logging to stderr, first emit a log message
+     * saying so, to provide a breadcrumb trail for users who may not remember
+     * that their logging is configured to go somewhere else.
+     */
+    if (!(Log_destination & LOG_DESTINATION_STDERR))
+        ereport(LOG,
+                (errmsg("ending log output to stderr"),
+                 errhint("Future log output will go to log destination \"%s\".",
+                         Log_destination_string)));
+
+    whereToSendOutput = DestNone;
+
+    /*
+     * Report server startup in log.  While we could emit this much earlier,
+     * it seems best to do so after starting the log collector, if we intend
+     * to use one.
+     */
     ereport(LOG,
             (errmsg("starting %s", PG_VERSION_STR)));

@@ -1173,135 +1306,6 @@ PostmasterMain(int argc, char *argv[])
         AddToDataDirLockFile(LOCK_FILE_LINE_LISTEN_ADDR, "");

     /*
-     * Set up shared memory and semaphores.
-     */
-    reset_shared(PostPortNumber);
-
-    /*
-     * Estimate number of openable files.  This must happen after setting up
-     * semaphores, because on some platforms semaphores count as open files.
-     */
-    set_max_safe_fds();
-
-    /*
-     * Set reference point for stack-depth checking.
-     */
-    set_stack_base();
-
-    /*
-     * Initialize pipe (or process handle on Windows) that allows children to
-     * wake up from sleep on postmaster death.
-     */
-    InitPostmasterDeathWatchHandle();
-
-#ifdef WIN32
-
-    /*
-     * Initialize I/O completion port used to deliver list of dead children.
-     */
-    win32ChildQueue = CreateIoCompletionPort(INVALID_HANDLE_VALUE, NULL, 0, 1);
-    if (win32ChildQueue == NULL)
-        ereport(FATAL,
-                (errmsg("could not create I/O completion port for child queue")));
-#endif
-
-    /*
-     * Record postmaster options.  We delay this till now to avoid recording
-     * bogus options (eg, NBuffers too high for available memory).
-     */
-    if (!CreateOptsFile(argc, argv, my_exec_path))
-        ExitPostmaster(1);
-
-#ifdef EXEC_BACKEND
-    /* Write out nondefault GUC settings for child processes to use */
-    write_nondefault_variables(PGC_POSTMASTER);
-#endif
-
-    /*
-     * Write the external PID file if requested
-     */
-    if (external_pid_file)
-    {
-        FILE       *fpidfile = fopen(external_pid_file, "w");
-
-        if (fpidfile)
-        {
-            fprintf(fpidfile, "%d\n", MyProcPid);
-            fclose(fpidfile);
-
-            /* Make PID file world readable */
-            if (chmod(external_pid_file, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH) != 0)
-                write_stderr("%s: could not change permissions of external PID file \"%s\": %s\n",
-                             progname, external_pid_file, strerror(errno));
-        }
-        else
-            write_stderr("%s: could not write external PID file \"%s\": %s\n",
-                         progname, external_pid_file, strerror(errno));
-
-        on_proc_exit(unlink_external_pid_file, 0);
-    }
-
-    /*
-     * Remove old temporary files.  At this point there can be no other
-     * Postgres processes running in this directory, so this should be safe.
-     */
-    RemovePgTempFiles();
-
-    /*
-     * Forcibly remove the files signaling a standby promotion request.
-     * Otherwise, the existence of those files triggers a promotion too early,
-     * whether a user wants that or not.
-     *
-     * This removal of files is usually unnecessary because they can exist
-     * only during a few moments during a standby promotion. However there is
-     * a race condition: if pg_ctl promote is executed and creates the files
-     * during a promotion, the files can stay around even after the server is
-     * brought up to new master. Then, if new standby starts by using the
-     * backup taken from that master, the files can exist at the server
-     * startup and should be removed in order to avoid an unexpected
-     * promotion.
-     *
-     * Note that promotion signal files need to be removed before the startup
-     * process is invoked. Because, after that, they can be used by
-     * postmaster's SIGUSR1 signal handler.
-     */
-    RemovePromoteSignalFiles();
-
-    /* Do the same for logrotate signal file */
-    RemoveLogrotateSignalFiles();
-
-    /* Remove any outdated file holding the current log filenames. */
-    if (unlink(LOG_METAINFO_DATAFILE) < 0 && errno != ENOENT)
-        ereport(LOG,
-                (errcode_for_file_access(),
-                 errmsg("could not remove file \"%s\": %m",
-                        LOG_METAINFO_DATAFILE)));
-
-    /*
-     * If enabled, start up syslogger collection subprocess
-     */
-    SysLoggerPID = SysLogger_Start();
-
-    /*
-     * Reset whereToSendOutput from DestDebug (its starting state) to
-     * DestNone. This stops ereport from sending log messages to stderr unless
-     * Log_destination permits.  We don't do this until the postmaster is
-     * fully launched, since startup failures may as well be reported to
-     * stderr.
-     *
-     * If we are in fact disabling logging to stderr, first emit a log message
-     * saying so, to provide a breadcrumb trail for users who may not remember
-     * that their logging is configured to go somewhere else.
-     */
-    if (!(Log_destination & LOG_DESTINATION_STDERR))
-        ereport(LOG,
-                (errmsg("ending log output to stderr"),
-                 errhint("Future log output will go to log destination \"%s\".",
-                         Log_destination_string)));
-
-    whereToSendOutput = DestNone;
-
-    /*
      * Initialize stats collection subsystem (this does NOT start the
      * collector process!)
      */