Re: [PATCH] Fixed creation of empty .log files during log rotation

On 10/29/24 09:08, Арсений Косицын wrote:
>
> Hi everyone!
>
> A situation has been discovered in which empty .log files are being created.
>
> --Reproduce:
>
> 1) You need to set the following parameters in the "postgresql.conf" file:
>
> log_destination = 'csvlog'
> logging_collector = on
> log_rotation_age = 1min
> log_directory = '/home/user/builds/postgresql/log'
>
> 2) After starting the server, two files are being created in the
> directory with logs:
>
> -rw------- 1 user user  1,4K oct 10 13:29 postgresql-2024-10-10_132907.csv
> -rw------- 1 user user  172 oct 10 13:29 postgresql-2024-10-10_132907.log
>
> 3) The .log file is created anyway, regardless of the log_destination
> parameter.
> The following message is displayed in the .log file (due to the fact
> that the log_destination
> parameter is set = 'csvlog'):
> |
> |2024-10-10 13:05:52.539 MSK [1629065] LOG:  ending log output to stderr
> 2024-10-10 13:05:52.539 MSK [1629065] HINT:  Future log output will go
> to log destination "csvlog".
>
> Next, the stderr stream is redirected to a .csv file.
>
> 4) Now, logs are rotated once a minute (due to the set log_rotation_age
> parameter). Moreover, all
> open log files are rotated, including the .log file that I wrote about
> above. New ones are being created
> .csv and .log files. Logs themselves are sent to .csv, and nothing else
> is output to .log
> and it remains empty:
>
> -rw------- 1 user user 1,4K oct 10 13:29 postgresql-2024-10-10_132907.csv
> -rw------- 1 user user  172 oct 10 13:29 postgresql-2024-10-10_132907.log
> -rw------- 1 user user 1,4K oct 10 13:30 postgresql-2024-10-10_133000.csv
> -rw------- 1 user user    0 oct 10 13:30 postgresql-2024-10-10_133000.log
>
> --Fix:
>
> To correct the situation, you can add a check for the "Log_destination"
> parameter in the "logfile_rotate()"
> function of the "syslogger.c" module. Then only the logs specified in
> this parameter will be rotated.Thisis doneinthe proposedpatch.
>

Yeah, creating all those files seems rather unnecessary. But wouldn't it
be easier to do the check in logfile_rotate_dest(), instead of for each
call? I think something like this would do the trick:

@@ -1292,6 +1292,9 @@ logfile_rotate_dest(bool time_based_rotation, int
size_rotation_for,
if (!time_based_rotation && (size_rotation_for & target_dest) == 0)
return true;

+ if ((Log_destination & target_dest) == 0)
+ return true;
+
/* file extension depends on the destination type */
if (target_dest == LOG_DESTINATION_STDERR)
logFileExt = NULL;

In fact, isn't it wrong to do the check outside? logfile_rotate_dest()
is responsible for closing the log files too, and with the check outside
we keep the first .log file open forever (lsof shows that).

FWIW my fix has the same issue, but IMO that just means the logic needs
to be more complex, but still in logfile_rotate_dest().

regards

--
Tomas Vondra