Date: Thu, 16 May 2019 10:38:08 +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: <20190516013808.GA1415@paquier.xyz>
References: <15804-3721117bf40fb654@postgresql.org>
 <27CD45B5-88F7-4A09-8B31-EFE41FC37FE1@crunchydata.com>
 <26332.1557863524@sss.pgh.pa.us>
 <20190515073125.GA1517@paquier.xyz>
 <4643.1557931189@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="H1spWtNR+x+ondvy"
Content-Disposition: inline
In-Reply-To: <4643.1557931189@sss.pgh.pa.us>
Precedence: bulk


--H1spWtNR+x+ondvy
Content-Type: multipart/mixed; boundary="y0ulUmNC+osPPQO6"
Content-Disposition: inline


--y0ulUmNC+osPPQO6
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Wed, May 15, 2019 at 10:39:49AM -0400, Tom Lane wrote:
> 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?
>=20
> 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.

My thoughts were to switch the LOG to DEBUG1 for the code paths where
we do not expect a failure, which is the code path from the
postmaster, so that actually comes closer to your upthread suggestion
of merging both functions.  The only thing which changes is that we
would call UnmapViewOfFile(), and fail silently.  We don't generate an
ERROR now for the existing code, just a LOG.  So there is no actual
hard failure now?  Anyway, you are right that it is no time to
redesign this interface with the current timing.

Using vcregress check with logging_collector and assertions enabled
is enough to see the failures on Windows, but the assertions actually
triggered are not the same ones as the EXEC_BACKEND case with sysv
because it's actually the three assertions in
pgwin32_ReserveSharedMemoryRegion() which blow up, not the ones in
PGSharedMemoryNoReAttach() which is able to handle its own.  Removing
some assertions are per the attached avoids problems.  I am able to
pass check with the logging collector enabled on Windows.

> 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,
>=20
> https://coverage.postgresql.org/src/backend/postmaster/syslogger.c.gcov.h=
tml
>=20
> 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?

Now from what I can see from the related buildfarm members:
- culicidae uses EXEC_BACKEND on Linux and has assertions, but not
logging_collector.
- dory, hamerkop, woodloose and bowerbird test on Windows with MSVC,
have assertions enabled, but not logging_collector.=20
- jacana tests on Windows with mingw, has assertions, but it does not
enforce logging_collector.
The Windows animals do not run bincheck.

So my bet is that the coverage is from pg_ctl/t/004_logrotate.pl,
which is a test skipped on Windows.  culicidae runs that, so the fact
that the failure got undetected is actually a bit of a mystery because
this uses sysv_shmem.c.

Except culicidae, I am seeing no members using EXEC_BACKEND which
enable the syslogger and use TAP tests :(
--
Michael

--y0ulUmNC+osPPQO6
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="shmem-remove-asserts.patch"
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/port/sysv_shmem.c b/src/backend/port/sysv_shmem.c
index fda3442c5b..5a1e782f69 100644
--- a/src/backend/port/sysv_shmem.c
+++ b/src/backend/port/sysv_shmem.c
@@ -845,7 +845,6 @@ PGSharedMemoryReAttach(void)
 void
 PGSharedMemoryNoReAttach(void)
 {
-	Assert(UsedShmemSegAddr !=3D NULL);
 	Assert(IsUnderPostmaster);
=20
 #ifdef __CYGWIN__
diff --git a/src/backend/port/win32_shmem.c b/src/backend/port/win32_shmem.c
index ccd7b6b5e3..5ae0643c20 100644
--- a/src/backend/port/win32_shmem.c
+++ b/src/backend/port/win32_shmem.c
@@ -551,10 +551,6 @@ pgwin32_ReserveSharedMemoryRegion(HANDLE hChild)
 {
 	void	   *address;
=20
-	Assert(ShmemProtectiveRegion !=3D NULL);
-	Assert(UsedShmemSegAddr !=3D NULL);
-	Assert(UsedShmemSegSize !=3D 0);
-
 	/* ShmemProtectiveRegion */
 	address =3D VirtualAllocEx(hChild, ShmemProtectiveRegion,
 							 PROTECTIVE_REGION_SIZE,

--y0ulUmNC+osPPQO6--

--H1spWtNR+x+ondvy
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAlzcvwAACgkQnvQgOdby
QH2ReA//cg/NlxYyNA087dGa5tycQWYURUZTFW/NSHiSdRjq4OvTxyqVoUK348PV
crTalgtYYvhv26n81wx0znlOr8r7IJdcPoeD9BIaz+RCzxLa8rePlFrCw8FLIfH9
HvcSZfAEGRWtoJYOOStgDSCn6gXmo7Oq+HNIcCQqgY4WHsGQ/48ihHRD5NsRhpa3
qPFafLvos7x4ft0KPBiFv8GAI59OEUZUCusG5WLJSN9wHboS0BgjxJP+yrZN2tnU
4+V8755x5npGK7htlVs2E6wdv5wDYC7sQy0ygZ1zYzNIm7pe+XXZbOxYeibEiu1v
bgezSuaLmo7erFdIDl72RkYGUQTeNi+auzohiWSKFr4YvEoeqTPnKdcpVMYPpIZQ
XyfnkHfwrirmePfP3TCWUoaCBQa6tnQuFaijADYy3Ye90h4pXmgKhOenaeqn3ctg
bqz0nyEIcBeBv+u7Q9dphWcZV1smF5Sc43Wp+0JNNjCOChEkMOiXVOngcCNFu4YF
+Cfyla42TKJS50rCDyPTAb9KS+2afadF7QwLHisyLMSPY2zvgWWCg2NAyqbp8k40
3X37EQjd7RrGsSxBHI0LQAdBUFqKKQsmCYXwashJFpejY/wH3fw4uBbMsgagBqKh
wSY5XBv1LNpfwsMEuXCW7+6eu+POfUZE/58ND+vzVgR11/ciu4k=
=0ll+
-----END PGP SIGNATURE-----

--H1spWtNR+x+ondvy--