public inbox for [email protected]
help / color / mirror / Atom feedFrom: Michael Paquier <[email protected]>
To: Sehrope Sarkuni <[email protected]>
Cc: Pg Hackers <[email protected]>
Cc: [email protected]
Subject: Re: Add jsonlog log_destination for JSON server logs
Date: Fri, 17 Sep 2021 10:36:04 +0900
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAH7T-apxdwf7=CHD_Vuo+QgtV6TEnLc0N0_SMTJ-a-jFdOBOvA@mail.gmail.com>
References: <CAH7T-aqswBM6JWe4pDehi1uOiufqe06DJWaU5=X7dDLyqUExHg@mail.gmail.com>
<[email protected]>
<CAH7T-aoDTnRo-U1Etw35bJiJZow7f2euC0Fh6nhSkSSxz333GA@mail.gmail.com>
<CAH7T-aqW0_yxuqznFkQbjdZz1ck+96Q2HzZHki6dsQbVHxzEPw@mail.gmail.com>
<[email protected]>
<[email protected]>
<[email protected]>
<CAH7T-areunm7CBDMLo8jMJ1fs=87eTqRmKOAY6CoEkexfdxEaQ@mail.gmail.com>
<[email protected]>
<CAH7T-apxdwf7=CHD_Vuo+QgtV6TEnLc0N0_SMTJ-a-jFdOBOvA@mail.gmail.com>
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
Attachments:
[application/pgp-signature] signature.asc (833B, ../[email protected]/2-signature.asc)
download
view thread (27+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: Re: Add jsonlog log_destination for JSON server logs
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox