On Thu, Sep 16, 2021 at 05:27:20PM -0400, Sehrope Sarkuni wrote:
> Attached three patches refactor the syslogger handling of file based
> destinations a bit ... and then a lot.
>
> First patch adds a new constant to represent both file based destinations.
> This should make it easier to ensure additional destinations are handled in
> "For all file destinations..." situations (e.g. when we add the jsonlog
> destination).
>
> Second patch refactors the file descriptor serialization and re-opening in
> EXEC_BACKEND forking. Previously the code was duplicated for both stderr
> and csvlog. Again, this should simplify adding new destinations as they'd
> just invoke the same helper. There's an existing comment about not handling
> failed opens in syslogger_parseArgs(...) and this patch doesn't fix that,
> but it does provide a single location to do so in the future.
>
> The third patch adds a new internal (to syslogger.c) structure,
> FileLogDestination, for file based log destinations and modifies the
> existing handling for syslogFile and csvlogFile to defer to a bunch of
> helper functions. It also renames "syslogFile" to "stderr_file_info".
> Working through this patch, it was initially confusing that the stderr log
> file was named "syslogFile", the C file is named syslogger.c, and there's
> an entirely separate syslog (the message logging API) destination. I think
> this clears that up a bit.
>
> The patches pass check-world on Linux. I haven't tested it on Windows but
> it does pass check-world with EXEC_BACKEND defined to try out the forking
> code paths. Definitely needs some review to ensure it's functionally
> correct for the different log files.
Compilation with EXEC_BACKEND on Linux should work, so no need to
bother with Windows when it comes to that. There is a buildfarm
machine testing this configuration, for example.
> I haven't tried splitting the csvlog code out or adding the new jsonlog
> atop this yet. There's enough changes here that I'd like to get this
> settled first.
Makes sense.
I am not really impressed by 0001 and the introduction of
LOG_DESTINATIONS_WITH_FILES. So I would leave that out and keep the
list of destinations listed instead. And we are talking about two
places here, only within syslogger.c.
0002 looks like an improvement, but I think that 0003 makes the
readability of the code worse with the introduction of eight static
routines to handle all those cases.
file_log_dest_should_rotate_for_size(), file_log_dest_close(),
file_log_dest_check_rotate_for_size(), get_syslogger_file() don't
bring major improvements. On the contrary,
file_log_dest_write_metadata and file_log_dest_rotate seem to reduce a
bit the noise.
--
Michael