Re: Patch to implement pg_current_logfile() function

Le 04/11/2016 à 14:17, Karl O. Pinc a écrit :
> 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.)
Yes, also if log_collector is on and log_destination is not stderr or
csvlog, current_logfiles exists too but it is emtpy.
When log_collector is off this file doesn't exists.

> 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.

Your picture is good, you just miss the case where we just change
log_destination. In this case, syslogger add/change log file destination
but rotation_requested is false. If write to current_logfiles is
conditional to this flag, it will never reflect the file change until
next rotation, see why next answer bellow.

If log_destination remain unchanged I agree that I am rewriting the same
information, but I don't think that this kind of sporadic I/O is enough
to append code to store the old log_destination value and check if there
is a change like what is done with Log_directory. This is my opinion,
but may be I'm wrong to consider that those isolated and not critical
I/O doesn't justify this work.

> 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.

Yes, but the problem here is that logfile_writename() doesn't write the
change because the test (Log_destination & LOG_DESTINATION_CSVLOG)
returns false, Log_destination is set after the file is successfully
created. This make me though that the call of logfile_writename() into
function open_csvlogfile() can be removed, thanks for pointing me that.
I attached a v12 patch with some comment fix in the call to
logfile_writename().

Regards,

--
Gilles Darold
Consultant PostgreSQL
http://dalibo.com - http://dalibo.org