Re: BUG #15804: Assertion failure when using logging_collector with EXEC_BACKEND

Michael Paquier <michael@paquier.xyz> writes:
> On Tue, May 14, 2019 at 03:52:04PM -0400, Tom Lane wrote:
>> It might be better to give up the assertion in PGSharedMemoryNoReAttach,
>> and just make it work more like PGSharedMemoryDetach, ie "detach if
>> UsedShmemSegAddr is set, else do nothing".  I don't remember for sure,
>> but if we do that, there might be no functional difference anymore
>> between those two functions, in which case we might as well merge 'em.

> It would be nice to fix that before beta1 is out, or we are going to
> get complaints :(

Agreed, that's why I put it on the open items list ;-)

> So what about dropping PGSharedMemoryNoReAttach() completely and using
> (or abusing of?) the detach routine so as we rely on its no-op
> behavior when a subprocess is not attached yet to a shared memory
> segment.  On Windows, the sub-processes calling NoReAttach don't map
> to a segment per the comments in the code, and I can indeed see some
> "could not map" LOG messages coming from these in the logs if
> enforcing the use of the detach routine in postmaster.c.  It could
> actually be an advantage to unmap using UnmapViewOfFile() so as a
> subsequent call of ReAttach does not finish by mapping an extra time.
> We may consider lowering the LOG messages to DEBUG1, or extend the
> detach routine so as we control the level of logs with an elevel to
> avoid sparse LOG messages when unmapping from shared segments where we
> know it would fail.

Hm.  I don't much like ignoring errors, which is what that would
amount to.  It seems like in win32_shmem.c, PGSharedMemoryNoReAttach
is already more or less right: resetting UsedShmemSegAddr to NULL and
then calling PGSharedMemoryDetach is exactly what to do.  It just needs
to lose the no-longer-correct assertions.  Maybe sysv_shmem.c's version
should look the same?

The real point of PGSharedMemoryNoReAttach, now that I think about it,
is that it's dealing with a case where we passed down UsedShmemSegAddr
via the BackendParameters mechanism, but we know that the segment is
not really attached (because of exec()).  This is different from the
API expectations of PGSharedMemoryDetach, which thinks that non-nullness
of UsedShmemSegAddr corresponds to whether the segment is attached or
not.  So we can't unify those functions without breaking things, or
at least losing the ability to distinguish errors from not-errors.

BTW, another thing I think is worth looking into is exactly why the
buildfarm failed to detect this problem.  According to the code
coverage report,

https://coverage.postgresql.org/src/backend/postmaster/syslogger.c.gcov.html

we *are* exercising the syslogger at some point during a standard
test run.  Perhaps the Windows animals don't run whatever test
case that is?

            regards, tom lane