Re: Patch to implement pg_current_logfile() function

On Mon, 31 Oct 2016 10:19:18 +0100
Gilles Darold <gilles(dot)darold(at)dalibo(dot)com> wrote:

> Le 31/10/2016 à 04:35, Karl O. Pinc a écrit :

> > Attached is a patch to apply on top of the v10 patch.
> >
> > It updates current_logfiles only once per log rotation.
> > I see no reason to open and write the file twice
> > if both csvlog and stderr logging is happening.
>
> I do not take the effort to do that because log rotation is not
> something that occurs too often and I don't want to change the
> conditional "time_based_rotation" code lines in logfile_rotate() for
> readability. My though was also that on high load, log rotation is
> automatically disabled so logfile_writename() is not called and there
> will be no additional I/O. But why not, if commiters want to save this
> additional I/O, this patch can be applied.

Ok. You didn't put this into your v11 patch so it can be submitted to
the committers as a separate patch.

> > I don't understand why you're calling
> > logfile_writename(last_file_name, last_csv_file_name);
> > in the SIGHUP code. Presumably you've already
> > written the old logfile names to current_logfiles.
> > On SIGHUP you want to write the new names, not
> > the old ones. But the point of the SIGHUP code
> > is to look for changes in logfile writing and then
> > call logfile_rotate() to make those changes. And
> > logfile_rotate() calls logfile_writename() as appropriate.
> > Shouldn't the logfile_writename call in the SIGHUP
> > code be eliminated?
>
> No, when you change the log_destination and reload configuration you
> have to refresh the content of current_logfiles, in this case no new
> rotation have been done and you have to write last_file_name and/or
> last_csv_file_name.

I don't understand. Please explain what's wrong with the
picture I have of how logging operates on receipt of SIGHUP.
Here is my understanding:

The system is running normally, current_logfiles exists and contains
the log file paths of the logs presently being written into. These
paths end with the file names in last_file_name and/or
last_csv_file_name. (I'm assuming throughout my description here that
log_destination is writing to the file system at all.)

A SIGHUP arrives. The signal handler, sigHupHandler(), sets the
got_SIGHUP flag. Nothing else happens.

The logging process wakes up due to the signal.
Either there's also log data or there's not. If there's
not:

The logging process goes through it's processing loop and finds,
at line 305 of syslogger.c, got_SIGHUP to be true.
Then it proceeds to do a bunch of assignment statements.
If it finds that the log directory or logfile name changed
it requests immediate log file rotation. It does this by
setting the request_rotation flag. If log_destination
or logging_collector changed request_rotation is not set.

Then, your patch adds a call to logfile_writename().
But nothing has touched the last_file_name or last_csv_file_name
variables. You rewrite into current_logfiles what's
already in current_logfiles.

If there is log data in the pipe on SIGHUP
and it's csv log data then if there's a csv
log file open that's the file that's written to.
last_csv_file_name does not change so current_logfiles
does not need to be re-written. If there is no csv
log file open then open_csvlogfile() is called
and it calls logfile_writename(). No need to
call logfile_writename() again when processing the
SIGHUP.

If there is log data in the pipe on SIGHUP
and it's stderr log data then the currently open
stderr log file is written to. This is already
in current_logfiles so no need to call logfile_writename().

Looking at what happens after your call to logfile_writename()
in SysLoggerMain() there's no changes to the log files
without calling logfile_writename.

If there's stderr
log messages in the pipe these get written to the currently
open stderr log file until log rotation changes the file
name. This either happens immediately because
request_rotation was set, or it waits.

If there's csv log messages in the pipe then these are either
written to the currently open log file or, when no
csv log file is open, open_csvlogfile() calls logfile_writename().
Either way, no need to re-write current_logfiles until
log rotation.

I'll respond to the rest of this email later, hopefully later
today.

Regards,

Karl <kop(at)meme(dot)com>
Free Software: "You don't pay back, you pay forward."
-- Robert A. Heinlein