Fixing syslogger rotation logic for first-time case

We've had a couple of complaints recently from people who were unhappy
because the syslogger's log_truncate_on_rotation logic does not fire
during the first log rotation after it's forked off from the postmaster.
The key reason for that was that to know whether to truncate or not,
the code has to know if the rotation actually changed to a new file
name, and it did not have that information inherited from the
postmaster.  The attached patch deals with that problem by passing down
the pg_time_t that the log file name is computed from, and then
reconstructing the file name.  This is kind of the hard way in Unix-oid
platforms: we could just let the malloc'd file name hang around through
the fork.  But on Windows it would be necessary to include the file name
in the BackendParameters struct that's built on every child process
launch, and that seemed pretty costly, considering the overwhelming
majority of postmaster children don't need it.  So I did it like this.

Any objections?

            regards, tom lane

diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index bbf725dfbc5aa58a5b43f15998e2c0424906ecfb..bfef707050a82f7842b1e2da550efcfa4882ec82 100644
*** a/src/backend/postmaster/postmaster.c
--- b/src/backend/postmaster/postmaster.c
*************** typedef struct
*** 441,446 ****
--- 441,447 ----
      pid_t        PostmasterPid;
      TimestampTz PgStartTime;
      TimestampTz PgReloadTime;
+     pg_time_t    first_syslogger_file_time;
      bool        redirection_done;
      bool        IsBinaryUpgrade;
      int            max_safe_fds;
*************** MaxLivePostmasterChildren(void)
*** 4701,4707 ****

  /*
   * The following need to be available to the save/restore_backend_variables
!  * functions
   */
  extern slock_t *ShmemLock;
  extern LWLock *LWLockArray;
--- 4702,4708 ----

  /*
   * The following need to be available to the save/restore_backend_variables
!  * functions.  They are marked NON_EXEC_STATIC in their home modules.
   */
  extern slock_t *ShmemLock;
  extern LWLock *LWLockArray;
*************** extern slock_t *ProcStructLock;
*** 4709,4714 ****
--- 4710,4716 ----
  extern PGPROC *AuxiliaryProcs;
  extern PMSignalData *PMSignalState;
  extern pgsocket pgStatSock;
+ extern pg_time_t first_syslogger_file_time;

  #ifndef WIN32
  #define write_inheritable_socket(dest, src, childpid) ((*(dest) = (src)), true)
*************** save_backend_variables(BackendParameters
*** 4761,4766 ****
--- 4763,4769 ----
      param->PostmasterPid = PostmasterPid;
      param->PgStartTime = PgStartTime;
      param->PgReloadTime = PgReloadTime;
+     param->first_syslogger_file_time = first_syslogger_file_time;

      param->redirection_done = redirection_done;
      param->IsBinaryUpgrade = IsBinaryUpgrade;
*************** restore_backend_variables(BackendParamet
*** 4985,4990 ****
--- 4988,4994 ----
      PostmasterPid = param->PostmasterPid;
      PgStartTime = param->PgStartTime;
      PgReloadTime = param->PgReloadTime;
+     first_syslogger_file_time = param->first_syslogger_file_time;

      redirection_done = param->redirection_done;
      IsBinaryUpgrade = param->IsBinaryUpgrade;
diff --git a/src/backend/postmaster/syslogger.c b/src/backend/postmaster/syslogger.c
index 919cc49fa945f27642f8723fefec486cc6657478..0febf64d87f2b337b70e4c9e49d4399b6aa03c0e 100644
*** a/src/backend/postmaster/syslogger.c
--- b/src/backend/postmaster/syslogger.c
***************
*** 2,8 ****
   *
   * syslogger.c
   *
!  * The system logger (syslogger) is new in Postgres 8.0. It catches all
   * stderr output from the postmaster, backends, and other subprocesses
   * by redirecting to a pipe, and writes it to a set of logfiles.
   * It's possible to have size and age limits for the logfile configured
--- 2,8 ----
   *
   * syslogger.c
   *
!  * The system logger (syslogger) appeared in Postgres 8.0. It catches all
   * stderr output from the postmaster, backends, and other subprocesses
   * by redirecting to a pipe, and writes it to a set of logfiles.
   * It's possible to have size and age limits for the logfile configured
*************** static bool pipe_eof_seen = false;
*** 91,96 ****
--- 91,97 ----
  static bool rotation_disabled = false;
  static FILE *syslogFile = NULL;
  static FILE *csvlogFile = NULL;
+ NON_EXEC_STATIC pg_time_t first_syslogger_file_time = 0;
  static char *last_file_name = NULL;
  static char *last_csv_file_name = NULL;
  static Latch sysLoggerLatch;
*************** SysLoggerMain(int argc, char *argv[])
*** 291,296 ****
--- 292,304 ----
          elog(FATAL, "could not create syslogger data transfer thread: %m");
  #endif   /* WIN32 */

+     /*
+      * Remember active logfile's name.  We recompute this from the reference
+      * time because passing down just the pg_time_t is a lot cheaper than
+      * passing a whole file path in the EXEC_BACKEND case.
+      */
+     last_file_name = logfile_getname(first_syslogger_file_time, NULL);
+
      /* remember active logfile parameters */
      currentLogDir = pstrdup(Log_directory);
      currentLogFilename = pstrdup(Log_filename);
*************** SysLogger_Start(void)
*** 560,568 ****

      /*
       * The initial logfile is created right in the postmaster, to verify that
!      * the Log_directory is writable.
       */
!     filename = logfile_getname(time(NULL), NULL);

      syslogFile = logfile_open(filename, "a", false);

--- 568,585 ----

      /*
       * The initial logfile is created right in the postmaster, to verify that
!      * the Log_directory is writable.  We save the reference time so that
!      * the syslogger child process can recompute this file name.
!      *
!      * It might look a bit strange to re-do this during a syslogger restart,
!      * but we must do so since the postmaster closed syslogFile after the
!      * previous fork (and remembering that old file wouldn't be right anyway).
!      * Note we always append here, we won't overwrite any existing file.  This
!      * is consistent with the normal rules, because by definition this is not
!      * a time-based rotation.
       */
!     first_syslogger_file_time = time(NULL);
!     filename = logfile_getname(first_syslogger_file_time, NULL);

      syslogFile = logfile_open(filename, "a", false);

*************** pipeThread(void *arg)
*** 1046,1053 ****
  #endif   /* WIN32 */

  /*
!  * open the csv log file - we do this opportunistically, because
   * we don't know if CSV logging will be wanted.
   */
  static void
  open_csvlogfile(void)
--- 1063,1074 ----
  #endif   /* WIN32 */

  /*
!  * Open the csv log file - we do this opportunistically, because
   * we don't know if CSV logging will be wanted.
+  *
+  * This is only used the first time we open the csv log in a given syslogger
+  * process, not during rotations.  As with opening the main log file, we
+  * always append in this situation.
   */
  static void
  open_csvlogfile(void)
*************** open_csvlogfile(void)
*** 1058,1064 ****

      csvlogFile = logfile_open(filename, "a", false);

!     pfree(filename);
  }

  /*
--- 1079,1088 ----

      csvlogFile = logfile_open(filename, "a", false);

!     if (last_csv_file_name != NULL)        /* probably shouldn't happen */
!         pfree(last_csv_file_name);
!
!     last_csv_file_name = filename;
  }

  /*
*************** logfile_rotate(bool time_based_rotation,
*** 1137,1150 ****
       * elapsed time and not something else, and (c) the computed file name is
       * different from what we were previously logging into.
       *
!      * Note: during the first rotation after forking off from the postmaster,
!      * last_file_name will be NULL.  (We don't bother to set it in the
!      * postmaster because it ain't gonna work in the EXEC_BACKEND case.) So we
!      * will always append in that situation, even though truncating would
!      * usually be safe.
!      *
!      * For consistency, we treat CSV logs the same even though they aren't
!      * opened in the postmaster.
       */
      if (time_based_rotation || (size_rotation_for & LOG_DESTINATION_STDERR))
      {
--- 1161,1167 ----
       * elapsed time and not something else, and (c) the computed file name is
       * different from what we were previously logging into.
       *
!      * Note: last_file_name should never be NULL here, but if it is, append.
       */
      if (time_based_rotation || (size_rotation_for & LOG_DESTINATION_STDERR))
      {