Date: Wed, 15 May 2019 16:31:25 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Yuli Khodorkovskiy <yuli.khodorkovskiy@crunchydata.com>,
	pgsql-bugs@lists.postgresql.org,
	Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
Subject: Re: BUG #15804: Assertion failure when using logging_collector with
 EXEC_BACKEND
Message-ID: <20190515073125.GA1517@paquier.xyz>
References: <15804-3721117bf40fb654@postgresql.org>
 <27CD45B5-88F7-4A09-8B31-EFE41FC37FE1@crunchydata.com>
 <26332.1557863524@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="uQr8t48UFsdbeI+V"
Content-Disposition: inline
In-Reply-To: <26332.1557863524@sss.pgh.pa.us>
Precedence: bulk


--uQr8t48UFsdbeI+V
Content-Type: multipart/mixed; boundary="ZPt4rx8FFjLCG7dd"
Content-Disposition: inline


--ZPt4rx8FFjLCG7dd
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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

--ZPt4rx8FFjLCG7dd
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="shmem-noattach.patch"
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/port/sysv_shmem.c b/src/backend/port/sysv_shmem.c
index fda3442c5b..8b70dab1a9 100644
--- a/src/backend/port/sysv_shmem.c
+++ b/src/backend/port/sysv_shmem.c
@@ -827,38 +827,6 @@ PGSharedMemoryReAttach(void)
 	UsedShmemSegAddr =3D hdr;		/* probably redundant */
 }
=20
-/*
- * PGSharedMemoryNoReAttach
- *
- * This is called during startup of a postmaster child process when we cho=
ose
- * *not* to re-attach to the existing shared memory segment.  We must clea=
n 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 PGSharedMemoryD=
etach
- * 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 !=3D 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 =3D NULL;
-	/* And the same for UsedShmemSegID. */
-	UsedShmemSegID =3D 0;
-}
-
 #endif							/* EXEC_BACKEND */
=20
 /*
diff --git a/src/backend/port/win32_shmem.c b/src/backend/port/win32_shmem.c
index ccd7b6b5e3..fa080222f6 100644
--- a/src/backend/port/win32_shmem.c
+++ b/src/backend/port/win32_shmem.c
@@ -431,41 +431,6 @@ PGSharedMemoryReAttach(void)
 	UsedShmemSegAddr =3D hdr;		/* probably redundant */
 }
=20
-/*
- * PGSharedMemoryNoReAttach
- *
- * This is called during startup of a postmaster child process when we cho=
ose
- * *not* to re-attach to the existing shared memory segment.  We must clea=
n up
- * to leave things in the appropriate state.
- *
- * The child process startup logic might or might not call PGSharedMemoryD=
etach
- * 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 !=3D NULL);
-	Assert(UsedShmemSegAddr !=3D 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 =3D 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/p=
ostmaster.c
index 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=3D", 15) =3D=3D 0)
 		PGSharedMemoryReAttach();
 	else
-		PGSharedMemoryNoReAttach();
+		PGSharedMemoryDetach();
=20
 	/* autovacuum needs this set before calling InitProcess */
 	if (strcmp(argv[1], "--forkavlauncher") =3D=3D 0)
diff --git a/src/include/storage/pg_shmem.h b/src/include/storage/pg_shmem.h
index 0535d4545c..88ad1c8205 100644
--- a/src/include/storage/pg_shmem.h
+++ b/src/include/storage/pg_shmem.h
@@ -79,7 +79,6 @@ extern void *UsedShmemSegAddr;
=20
 #ifdef EXEC_BACKEND
 extern void PGSharedMemoryReAttach(void);
-extern void PGSharedMemoryNoReAttach(void);
 #endif
=20
 extern PGShmemHeader *PGSharedMemoryCreate(Size size, int port,

--ZPt4rx8FFjLCG7dd--

--uQr8t48UFsdbeI+V
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAlzbwE0ACgkQnvQgOdby
QH1VZBAAkzp2z5+JDJ2VaeX+6vj8ScTJocaIpYi+ZhjQXUny3l798wv7a7XP5MU5
gcv8GpQLvUwJVRJTO2EHtVYKmxp1d5GdpWI6hzTDg4eYHj4uzgzq69UhGv2q54wF
B7+VwKtn2O08ZNlJ8H/KsZUz1FS9nv7uFZWayfHI7wfAsodKH3yQsaseff21YgeO
RjDuYzZutw1JGO1NHDuqwmHD/1EdWWhryIEfq/i8lDsCdxB3WZojgycwF+/s82rp
AW/jB/Ueq6ApF2oLOxqVw/tiUA0HCa+nduE4XqqSATtsuHCy7REvbEHE8E7FQdwS
ua28H5XAXRgdf3Ym55/ANLs7NoVcDOYqqrTshwKMrwfoSOlcYqqMB7UmIK658XPz
6gE8V7tVxANByccle6UxcGGl8gyT0PO97JOdBvC66sLOxQ9wppL4+THB1QnLUYDS
m4E4qZ3Crn5AkX3bvmYXUjNlAX+7S4JZ5tb0ovsSRi2g5cIQpZVLjOYbOxRRjj6j
uE2vWvm/9zsMYcfRxAa9NFUNr0VmXZYv+dbKhOf9HXjW+oLFbWu2m5CQcSgdNrky
YEwj+IDrZvmmCxYInZsaUv6Mh0mAC0M69sNdY1jf6JaiwMXvWDVZjPu+kaxXsiD1
QXZeEtFaXunyxGUuWlG/Dsa8vMAlxe6rIjw5Gp0hWRFMFXP+ytQ=
=t0JI
-----END PGP SIGNATURE-----

--uQr8t48UFsdbeI+V--