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

On Fri, Dec 13, 2024 at 02:21:45PM +0300, Arseny Kositsin wrote:
> 08.12.2024 01:06, Tomas Vondra wrote:
>> 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().

Yep. All destination checks should be inside logfile_rotate_dest().
That's way cleaner to isolate which files to close and/or open when
the destinations switch.

> You can close the first file if you remove the second part of the condition
> in if (because it is stderr). I checked it in lsof and the first log file is
> closing:
>
> @@ -1274,8 +1274,7 @@ logfile_rotate_dest(bool time_based_rotation, int
> size_rotation_for,
>       * is assumed to be always opened even if stderr is disabled in
>       * log_destination.
>       */
> -    if ((Log_destination & target_dest) == 0 &&
> -        target_dest != LOG_DESTINATION_STDERR)
> +    if ((Log_destination & target_dest) == 0)
>   {
>     if (*logFile != NULL)
>       fclose(*logFile);
>
> But the comment above says that stderr should always remain open, even if
> disabled in log_destination. Is it really necessary to do more complex logic
> in order to close this file? I'm sorry if I misunderstood something.

If you do that the stderr log file would be closed when the stderr
destination is disabled.

> What do you think about this?

The comment you are pointed to has been added in 5c6e33f07153 (in
15~), but keeping the logfile fd for stderr opened all the time is
something historically much older than that so as, AFAIK, it can act
as a backup destination even if the syslogger is not started yet for
the postmaster or some of the other destination file could not be
opened. See also bff84a547d71, that has added a second comment in
write_syslogger_file() telling a few more details about the reason
why. The bug fixed by this commit is not "that" old, so you cannot do
what you are suggesting.

It's true that some of these files are useless to keep around when
there is an aggressive rotation, but does it really matter? I am
sure that there are many external tools that browse the contents of
the log directory and perform a cleanup of past files based on their
timestamp and the file name formats (like daily cleanups, etc). So
there is the argument that not rotating the stderr files would cause
harm as some files to actually keep would be removed by such tools.
--
Michael