Feedback-ID: i0fe9450f:Fastmail
Date: Mon, 8 Apr 2024 16:33:48 +0900
From: Michael Paquier <michael@paquier.xyz>
To: "Andrey M. Borodin" <x4mmm@yandex-team.ru>
Cc: Heikki Linnakangas <hlinnaka@iki.fi>, Tom Lane <tgl@sss.pgh.pa.us>,
	pgsql-hackers <pgsql-hackers@postgresql.org>,
	Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Subject: Re: Weird test mixup
Message-ID: <ZhOd3NXAutteokGL@paquier.xyz>
References: <2387221.1710454778@sss.pgh.pa.us>
 <ZfOABU4ahF2QZQ6N@paquier.xyz>
 <2393552.1710458033@sss.pgh.pa.us>
 <ZfP7IDs9TvrKe49x@paquier.xyz>
 <e1a7ab78-1fa9-419c-b0d1-36d971bdf946@iki.fi>
 <ZfTcZW1-Kh3fFLnZ@paquier.xyz>
 <ZfeTLfltCTimaVbU@paquier.xyz>
 <Zg9frr5iIam3fhd7@paquier.xyz>
 <FB71B3B1-D68F-4508-BA95-D427792FAB2B@yandex-team.ru>
 <ZhNG4Io9uYOgwv3F@paquier.xyz>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="sY1bG4PTCQsPDB5M"
Content-Disposition: inline
In-Reply-To: <ZhNG4Io9uYOgwv3F@paquier.xyz>
Archived-At: 
 <https://www.postgresql.org/message-id/ZhOd3NXAutteokGL%40paquier.xyz>
Precedence: bulk


--sY1bG4PTCQsPDB5M
Content-Type: multipart/mixed; boundary="L9rAIS5jp/gNPi9I"
Content-Disposition: inline


--L9rAIS5jp/gNPi9I
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Mon, Apr 08, 2024 at 10:22:40AM +0900, Michael Paquier wrote:
> For now I have applied 997db123c054 to make the GIN tests with
> injection points repeatable as it was an independent issue, and
> f587338dec87 to add the local function pieces.

Bharath has reported me offlist that one of the new tests has a race
condition when doing the reconnection.  When the backend creating the
local points is very slow to exit, the backend created after the
reconnection may detect that a local point previously created still
exists, causing a failure.  The failure can be reproduced with a sleep
in the shmem exit callback, like:
--- a/src/test/modules/injection_points/injection_points.c
+++ b/src/test/modules/injection_points/injection_points.c
@@ -163,6 +163,8 @@ injection_points_cleanup(int code, Datum arg)
 	if (!injection_point_local)
 		return;
=20
+	pg_usleep(1000000 * 1L);
+
 	SpinLockAcquire(&inj_state->lock);
 	for (int i =3D 0; i < INJ_MAX_CONDITION; i++)
 	{

At first I was looking at a loop with a scan of pg_stat_activity, but
I've noticed that regress.so includes a wait_pid() that we can use to
make sure that a given process exits before moving on to the next
parts of a test, so I propose to just reuse that here.  This requires
tweaks with --dlpath for meson and ./configure, nothing new.  The CI
is clean.  Patch attached.

Thoughts?
--
Michael

--L9rAIS5jp/gNPi9I
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="0001-Stabilize-injection-point-test.patch"
Content-Transfer-Encoding: quoted-printable

=46rom 8f43807000c1f33a0238d7bbcc148a196e4134e4 Mon Sep 17 00:00:00 2001
=46rom: Michael Paquier <michael@paquier.xyz>
Date: Mon, 8 Apr 2024 16:28:17 +0900
Subject: [PATCH] Stabilize injection point test

---
 src/test/modules/injection_points/Makefile       |  1 +
 .../expected/injection_points.out                | 16 ++++++++++++++++
 src/test/modules/injection_points/meson.build    |  1 +
 .../injection_points/sql/injection_points.sql    | 15 +++++++++++++++
 4 files changed, 33 insertions(+)

diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/=
injection_points/Makefile
index 1cb395c37a..31bd787994 100644
--- a/src/test/modules/injection_points/Makefile
+++ b/src/test/modules/injection_points/Makefile
@@ -7,6 +7,7 @@ DATA =3D injection_points--1.0.sql
 PGFILEDESC =3D "injection_points - facility for injection points"
=20
 REGRESS =3D injection_points
+REGRESS_OPTS =3D --dlpath=3D$(top_builddir)/src/test/regress
=20
 # The injection points are cluster-wide, so disable installcheck
 NO_INSTALLCHECK =3D 1
diff --git a/src/test/modules/injection_points/expected/injection_points.ou=
t b/src/test/modules/injection_points/expected/injection_points.out
index 3d94016ac9..d2a69b54e8 100644
--- a/src/test/modules/injection_points/expected/injection_points.out
+++ b/src/test/modules/injection_points/expected/injection_points.out
@@ -1,4 +1,11 @@
 CREATE EXTENSION injection_points;
+\getenv libdir PG_LIBDIR
+\getenv dlsuffix PG_DLSUFFIX
+\set regresslib :libdir '/regress' :dlsuffix
+CREATE FUNCTION wait_pid(int)
+  RETURNS void
+  AS :'regresslib'
+  LANGUAGE C STRICT;
 SELECT injection_points_attach('TestInjectionBooh', 'booh');
 ERROR:  incorrect action "booh" for injection point creation
 SELECT injection_points_attach('TestInjectionError', 'error');
@@ -156,8 +163,17 @@ NOTICE:  notice triggered for injection point TestCond=
itionLocal2
 =20
 (1 row)
=20
+select pg_backend_pid() as oldpid \gset
 -- reload, local injection points should be gone.
 \c
+-- Wait for the previous backend process to exit, ensuring that its local
+-- injection points are cleaned up.
+SELECT wait_pid(:'oldpid');
+ wait_pid=20
+----------
+=20
+(1 row)
+
 SELECT injection_points_run('TestConditionLocal1'); -- nothing
  injection_points_run=20
 ----------------------
diff --git a/src/test/modules/injection_points/meson.build b/src/test/modul=
es/injection_points/meson.build
index a29217f75f..8e1b5b4539 100644
--- a/src/test/modules/injection_points/meson.build
+++ b/src/test/modules/injection_points/meson.build
@@ -33,6 +33,7 @@ tests +=3D {
     'sql': [
       'injection_points',
     ],
+    'regress_args': ['--dlpath', meson.build_root() / 'src/test/regress'],
     # The injection points are cluster-wide, so disable installcheck
     'runningcheck': false,
   },
diff --git a/src/test/modules/injection_points/sql/injection_points.sql b/s=
rc/test/modules/injection_points/sql/injection_points.sql
index 2aa76a542b..e18146eb8c 100644
--- a/src/test/modules/injection_points/sql/injection_points.sql
+++ b/src/test/modules/injection_points/sql/injection_points.sql
@@ -1,5 +1,14 @@
 CREATE EXTENSION injection_points;
=20
+\getenv libdir PG_LIBDIR
+\getenv dlsuffix PG_DLSUFFIX
+\set regresslib :libdir '/regress' :dlsuffix
+
+CREATE FUNCTION wait_pid(int)
+  RETURNS void
+  AS :'regresslib'
+  LANGUAGE C STRICT;
+
 SELECT injection_points_attach('TestInjectionBooh', 'booh');
 SELECT injection_points_attach('TestInjectionError', 'error');
 SELECT injection_points_attach('TestInjectionLog', 'notice');
@@ -40,8 +49,14 @@ SELECT injection_points_attach('TestConditionLocal1', 'e=
rror');
 SELECT injection_points_attach('TestConditionLocal2', 'notice');
 SELECT injection_points_run('TestConditionLocal1'); -- error
 SELECT injection_points_run('TestConditionLocal2'); -- notice
+
+select pg_backend_pid() as oldpid \gset
+
 -- reload, local injection points should be gone.
 \c
+-- Wait for the previous backend process to exit, ensuring that its local
+-- injection points are cleaned up.
+SELECT wait_pid(:'oldpid');
 SELECT injection_points_run('TestConditionLocal1'); -- nothing
 SELECT injection_points_run('TestConditionLocal2'); -- nothing
 SELECT injection_points_run('TestConditionError'); -- error
--=20
2.43.0


--L9rAIS5jp/gNPi9I--

--sY1bG4PTCQsPDB5M
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmYTndwACgkQnvQgOdby
QH30Aw/8CMql8ecmzv5ZXZ9VKTEP6o1MqhPc+nGsaT630vvG1QiDSAH8vXTcUPAp
JonfB4UtGA3W8LayKSJHLNqDb+yWgFGZpa/0Sy2PIVscmSBOwzRiJkljQc6+rjoO
D34BfIV+g2D6TUI+ZF/AJfisBzx48KJJGenb6bq1HIFxhT8ZWLgG0D+Q9No1cD4e
mwTbk4bQlE1G/uCAAFjIaSIOsV07S2liz7rb0dD22ue6qayiPeQV1g7JQ8ryU56x
+O5aWQ1AEEhhufq7PlBJzn/wZpI1u3b4uaseHckl/+3d/zGwfkNC2PdYG1+ou8Ec
u+6ZEbdrgsQNnxV5FN37HMEdGzUmldymtKESjEkA9MrotPXY+9xJfxzO3v7UDdSl
wQsKH7X68tasAhb4GmFMzD9PoVlLA2DHpHpNcYYJYaxRCofEeMEY9Dh1Ud5lWAzQ
65bAjjH0c2wMOygcG750iIf5h93cpVuT9GE73grfywcQ5p3fYv+jtJZfTLJQVwiF
ck5ymhQOu6ALW9egXJ6slHrQxqS1vsyM3jShDZ7rekErVge9yLdeKHH6/A+lv+yQ
OYxHyEdwhqTPVBDgTdkE8ncSOgzj9b++1R0M/kH8VcGXbx+ApJsAwJG/8mJfDsvd
4cblKkYwQ0mZER8L9B6+uRzfPbHW+7lcZu/xtjBknQFxAPK0LTg=
=tzKZ
-----END PGP SIGNATURE-----

--sY1bG4PTCQsPDB5M--