Re: Patch to implement pg_current_logfile() function
От | Gilles Darold |
---|---|
Тема | Re: Patch to implement pg_current_logfile() function |
Дата | |
Msg-id | 9cdf63c9-6f62-2d41-f2eb-3f225c93872c@dalibo.com обсуждение исходный текст |
Ответ на | Re: Patch to implement pg_current_logfile() function ("Karl O. Pinc" <kop@meme.com>) |
Ответы |
Re: Patch to implement pg_current_logfile() function
|
Список | pgsql-hackers |
Le 04/11/2016 à 14:17, Karl O. Pinc a écrit : > On Mon, 31 Oct 2016 10:19:18 +0100 > Gilles Darold <gilles.darold@dalibo.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
Вложения
В списке pgsql-hackers по дате отправления:
Предыдущее
От: Peter EisentrautДата:
Сообщение: Re: Password identifiers, protocol aging and SCRAM protocol