From: Michael Paquier <michael@paquier.xyz>
To: Tom Lane <tgl@sss.pgh.pa.us>
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: Wed, 15 May 2019 16:31:25 +0900
Message-ID: <20190515073125.GA1517@paquier.xyz> (raw)
In-Reply-To: <26332.1557863524@sss.pgh.pa.us>
References: <15804-3721117bf40fb654@postgresql.org>
<27CD45B5-88F7-4A09-8B31-EFE41FC37FE1@crunchydata.com>
<26332.1557863524@sss.pgh.pa.us>
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 :(
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.
I have been playing with the attached using EXEC_BACKEND on Linux and
some Windows builds FWIW. Thoughts?
--
Michael
Attachments:
[text/x-diff] shmem-noattach.patch (4.3K, ../20190515073125.GA1517@paquier.xyz/2-shmem-noattach.patch)
download | inline diff:diff --git a/src/backend/port/sysv_shmem.c b/src/backend/port/sysv_shmem.cindex fda3442c5b..8b70dab1a9 100644--- a/src/backend/port/sysv_shmem.c+++ b/src/backend/port/sysv_shmem.c@@ -827,38 +827,6 @@ PGSharedMemoryReAttach(void)
UsedShmemSegAddr = hdr; /* probably redundant */
}
-/*- * PGSharedMemoryNoReAttach- *- * This is called during startup of a postmaster child process when we choose- * *not* to re-attach to the existing shared memory segment. We must clean up- * to leave things in the appropriate state. This is not used in the non- * EXEC_BACKEND case, either.- *- * The child process startup logic might or might not call PGSharedMemoryDetach- * after this; make sure that it will be a no-op if called.- *- * UsedShmemSegID and UsedShmemSegAddr are implicit parameters to this- * routine. The caller must have already restored them to the postmaster's- * values.- */-void-PGSharedMemoryNoReAttach(void)-{- Assert(UsedShmemSegAddr != NULL);- Assert(IsUnderPostmaster);--#ifdef __CYGWIN__- /* cygipc (currently) appears to not detach on exec. */- PGSharedMemoryDetach();-#endif-- /* For cleanliness, reset UsedShmemSegAddr to show we're not attached. */- UsedShmemSegAddr = NULL;- /* And the same for UsedShmemSegID. */- UsedShmemSegID = 0;-}-
#endif /* EXEC_BACKEND */
/*
diff --git a/src/backend/port/win32_shmem.c b/src/backend/port/win32_shmem.cindex ccd7b6b5e3..fa080222f6 100644--- a/src/backend/port/win32_shmem.c+++ b/src/backend/port/win32_shmem.c@@ -431,41 +431,6 @@ PGSharedMemoryReAttach(void)
UsedShmemSegAddr = hdr; /* probably redundant */
}
-/*- * PGSharedMemoryNoReAttach- *- * This is called during startup of a postmaster child process when we choose- * *not* to re-attach to the existing shared memory segment. We must clean up- * to leave things in the appropriate state.- *- * The child process startup logic might or might not call PGSharedMemoryDetach- * after this; make sure that it will be a no-op if called.- *- * ShmemProtectiveRegion, UsedShmemSegID and UsedShmemSegAddr are implicit- * parameters to this routine. The caller must have already restored them to- * the postmaster's values.- */-void-PGSharedMemoryNoReAttach(void)-{- Assert(ShmemProtectiveRegion != NULL);- Assert(UsedShmemSegAddr != NULL);- Assert(IsUnderPostmaster);-- /*- * Under Windows we will not have mapped the segment, so we don't need to- * un-map it. Just reset UsedShmemSegAddr to show we're not attached.- */- UsedShmemSegAddr = NULL;-- /*- * We *must* close the inherited shmem segment handle, else Windows will- * consider the existence of this process to mean it can't release the- * shmem segment yet. We can now use PGSharedMemoryDetach to do that.- */- PGSharedMemoryDetach();-}-
/*
* PGSharedMemoryDetach
*
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.cindex 71ffb1345b..a10a9e0909 100644--- a/src/backend/postmaster/postmaster.c+++ b/src/backend/postmaster/postmaster.c@@ -4834,8 +4834,8 @@ SubPostmasterMain(int argc, char *argv[])
/*
* If appropriate, physically re-attach to shared memory segment. We want
* to do this before going any further to ensure that we can attach at the
- * same address the postmaster used. On the other hand, if we choose not- * to re-attach, we may have other cleanup to do.+ * same address the postmaster used. On the other hand, if we choose to+ * detach, get rid of any shared memory segment inherited previously.
*
* If testing EXEC_BACKEND on Linux, you should run this as root before
* starting the postmaster:
@@ -4854,7 +4854,7 @@ SubPostmasterMain(int argc, char *argv[])
strncmp(argv[1], "--forkbgworker=", 15) == 0)
PGSharedMemoryReAttach();
else
- PGSharedMemoryNoReAttach();+ PGSharedMemoryDetach();
/* autovacuum needs this set before calling InitProcess */
if (strcmp(argv[1], "--forkavlauncher") == 0)
diff --git a/src/include/storage/pg_shmem.h b/src/include/storage/pg_shmem.hindex 0535d4545c..88ad1c8205 100644--- a/src/include/storage/pg_shmem.h+++ b/src/include/storage/pg_shmem.h@@ -79,7 +79,6 @@ extern void *UsedShmemSegAddr;
#ifdef EXEC_BACKEND
extern void PGSharedMemoryReAttach(void);
-extern void PGSharedMemoryNoReAttach(void);
#endif
extern PGShmemHeader *PGSharedMemoryCreate(Size size, int port,
[application/pgp-signature] signature.asc (832B, ../20190515073125.GA1517@paquier.xyz/3-signature.asc)
download
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: michael@paquier.xyz, tgl@sss.pgh.pa.us, yuli.khodorkovskiy@crunchydata.com, peter.eisentraut@2ndquadrant.com
Subject: Re: BUG #15804: Assertion failure when using logging_collector with EXEC_BACKEND
In-Reply-To: <20190515073125.GA1517@paquier.xyz>
* 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