public inbox for [email protected]
help / color / mirror / Atom feedFrom: Michael Paquier <[email protected]>
To: Euler Taveira <[email protected]>
Cc: Postgres hackers <[email protected]>
Cc: Álvaro Herrera <[email protected]>
Subject: Re: NULL pointer dereference in syslogger with load_libraries() and -DEXEC_BACKEND at startup
Date: Tue, 26 May 2026 14:20:52 +0900
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<[email protected]>
<[email protected]>
On Mon, May 25, 2026 at 11:39:54PM -0300, Euler Taveira wrote:
> It seems I was too optimistic about this patch. Since the commit 0c8e082fba8
> sets MyBackendType to B_LOGGER earlier, it breaks the following assumption in
> syslogger.c.
>
> /*
> * If we're told to write to a structured log file, but it's not open,
> * dump the data to syslogFile (which is always open) instead. This can
> * happen if structured output is enabled after postmaster start and we've
> * been unable to open logFile. There are also race conditions during a
> * parameter change whereby backends might send us structured output
> * before we open the logFile or after we close it. Writing formatted
> * output to the regular log file isn't great, but it beats dropping log
> * output on the floor.
>
> It shouldn't assume syslogFile is always open. The send_message_to_server_log()
> shouldn't be executing the following code path in this case.
This comment has been rewritten in 2022 when jsonlog was added in
dc686681e079, but this assumption is much older than that:
bff84a547d71, where the syslogger has been made more robust. And we
assume that the log file is always open, so this change is breaking
an old assumption.
> It could set MyBackendType only if child_type != B_LOGGER during
> launch_backend.c and set it at the same code path as in the past. However, I
> consider this solution ugly and hackish.
While thinking about an approach that could allow to keep
0c8e082fba8d, I was wondering whether we should have a boolean flag
that tracks if the log file is opened or not that gets set (we should
not care about the reset) when we are done with its creation, but I'm
feeling that this makes the logic feeble. We know only rely on
MyBackendType for the job which means to complicate all these checks.
The part that makes me uneasy is that the logging facility should be
robust by design, and simpler is always better IMO.
An exception where we don't set MyBackendType and have an exception
for this corresponding child_type value does not really feel right to
me either.. As a whole, I am not sure to like what has been done
here.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, 2-signature.asc)
download
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], [email protected]
Subject: Re: NULL pointer dereference in syslogger with load_libraries() and -DEXEC_BACKEND at startup
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