From: Tom Lane <tgl@sss.pgh.pa.us>
To: Michael Paquier <michael@paquier.xyz>
Cc: Yuli Khodorkovskiy <yuli.khodorkovskiy@crunchydata.com>
Cc: pgsql-bugs@lists.postgresql.org, Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
Subject: Re: BUG #15804: Assertion failure when using logging_collector with EXEC_BACKEND
Date: Sat, 18 May 2019 13:07:15 -0400
Message-ID: <31224.1558199235@sss.pgh.pa.us> (raw)
In-Reply-To: <20190517042536.GM20887@paquier.xyz>
References: <26332.1557863524@sss.pgh.pa.us>
<20190515073125.GA1517@paquier.xyz>
<4643.1557931189@sss.pgh.pa.us>
<20190516013808.GA1415@paquier.xyz>
<20190516021903.GB1415@paquier.xyz>
<28927.1557977558@sss.pgh.pa.us>
<20190516035617.GC1415@paquier.xyz>
<20190516043618.GD1415@paquier.xyz>
<31437.1557982294@sss.pgh.pa.us>
<20190516060628.GE1415@paquier.xyz>
<20190517042536.GM20887@paquier.xyz>
[ some notes for whoever tries to fix this in the future ]
Michael Paquier <michael@paquier.xyz> writes:
> Trying to avoid calling pgwin32_ReserveSharedMemoryRegion() for
> processes which are never going to connect to shared memory is
> actually an interesting option, because this way there is no need to
> relax the assertions it includes,
Yeah, I'd come to the same conclusion while thinking about this,
but I don't think you're going about it in quite the right way.
In the first place, it's a mistake to try to drive the what-to-do-about-
shared-memory decision strictly off the type of the subprocess. In the
case of the syslogger, in particular, we have not only the initial launch
to think about but also a possible relaunch if the first syslogger child
crashes. In that scenario there *would* be shared memory to detach from.
So the sketch that's emerging in my mind is
1. The postmaster should have its own state variable, say
"static bool shared_memory_exists", which it sets at the appropriate
time. (We could rely on testing UsedShmemSegAddr for that, but I don't
think that'd be good design; UsedShmemSegAddr is really private state of
the particular shmem implementation.) Since internal_forkexec is
inside postmaster.c and runs in the postmaster process, it could just
test that state variable directly to see if it needs to call
pgwin32_ReserveSharedMemoryRegion.
2. I think we'd be well advised to get rid of the process-type-specific
tests in SubPostmasterMain, too. The idea in my mind is to have the
postmaster pass down an additional switch explicitly saying what to do
about shared memory, so that what SubPostmasterMain does is roughly
if (strcmp(argv[2], "--reattach") == 0)
PGSharedMemoryReAttach();
else if (strcmp(argv[2], "--noreattach") == 0)
PGSharedMemoryNoReAttach();
else
/* do nothing, shared memory does not exist yet */ ;
Then the problem for the syslogger is solved by passing --noreattach
or not depending on whether shared_memory_exists is true yet.
I've not worked that out in any detail; getting the switches included
properly might be too much of a pain for this to be an improvement.
But for sure I don't want there to be multiple copies of that list of
which subprocess types use shared memory.
A variant idea is to include shared_memory_exists in what's passed down
by the BackendParameters mechanism, and then subprocesses can adjust
their behavior for themselves.
In any case, the thrust of all of this is that we shouldn't touch any
of the assertions in the shmem support files; rather, the way to fix
it is to improve the logic in postmaster.c so that we don't call those
functions at all in child processes that are launched before shmem
exists.
regards, tom lane
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: pgsql-bugs@postgresql.org
Cc: tgl@sss.pgh.pa.us, michael@paquier.xyz, yuli.khodorkovskiy@crunchydata.com, peter.eisentraut@2ndquadrant.com
Subject: Re: BUG #15804: Assertion failure when using logging_collector with EXEC_BACKEND
In-Reply-To: <31224.1558199235@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox