Re: BUG #15804: Assertion failure when using logging_collector with EXEC_BACKEND - Mailing list pgsql-bugs

Michael Paquier <michael@paquier.xyz> writes:
> On Tue, Sep 10, 2019 at 01:28:51AM -0400, Tom Lane wrote:
>> But the external PID file is a different story.  I can believe that
>> something might be expecting the sockets to be open before we create
>> that file --- and since we're not using that file for an interlock,
>> there's no reason to be in a hurry to create it.  I think it'd make
>> sense to move that step further down, so it's still done after the
>> create-sockets step.

> That's the actual take here.  We cannot assume that nobody is relying
> on that.  If they do, it could be tricky to rework this logic.  Moving
> the external file write a bit later does not sound like a bad thing in
> itself to keep more consistency with the past.

Here's a version that does it like that.  After studying things a bit,
it seemed like a good idea to also move RemovePgTempFiles() down,
so that that possibly-slow step still occurs after making the external
PID file.  Everything else that we've moved to before that step
should be reasonably quick.

            regards, tom lane

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

-    /* Report server startup in log */
+    /*
+     * Set up shared memory and semaphores.
+     */
+    reset_shared();
+
+    /*
+     * 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
+
+#ifdef EXEC_BACKEND
+    /* Write out nondefault GUC settings for child processes to use */
+    write_nondefault_variables(PGC_POSTMASTER);
+#endif
+
+    /*
+     * 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,50 +1269,12 @@ PostmasterMain(int argc, char *argv[])
         AddToDataDirLockFile(LOCK_FILE_LINE_LISTEN_ADDR, "");

     /*
-     * Set up shared memory and semaphores.
-     */
-    reset_shared();
-
-    /*
-     * 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).
+     * bogus options (eg, unusable port number).
      */
     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
      */
@@ -1248,60 +1306,6 @@ PostmasterMain(int argc, char *argv[])
     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!)
      */
diff --git a/src/include/utils/pidfile.h b/src/include/utils/pidfile.h
index b2dcca6..21e7ccf 100644
--- a/src/include/utils/pidfile.h
+++ b/src/include/utils/pidfile.h
@@ -28,7 +28,8 @@
  *
  * Lines 6 and up are added via AddToDataDirLockFile() after initial file
  * creation; also, line 5 is initially empty and is changed after the first
- * Unix socket is opened.
+ * Unix socket is opened.  Onlookers should not assume that lines 4 and up
+ * are filled in any particular order.
  *
  * Socket lock file(s), if used, have the same contents as lines 1-5, with
  * line 5 being their own directory.