[Michael Paquier <michael@paquier.xyz>]

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:
inline patch 1: src/test/modules/injection_points/injection_points.c (+2/-0 lines) download inline-1.patch--- 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;
 
+	pg_usleep(1000000 * 1L);
+
 	SpinLockAcquire(&inj_state->lock);
 	for (int i = 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

Patches and attachments: download all (3 files, .tar.gz)

0001-Stabilize-injection-point-test.patch (text/x-diff, 4.1K) download
From 8f43807000c1f33a0238d7bbcc148a196e4134e4 Mon Sep 17 00:00:00 2001
From: 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 = injection_points--1.0.sql
 PGFILEDESC = "injection_points - facility for injection points"
 
 REGRESS = injection_points
+REGRESS_OPTS = --dlpath=$(top_builddir)/src/test/regress
 
 # The injection points are cluster-wide, so disable installcheck
 NO_INSTALLCHECK = 1
diff --git a/src/test/modules/injection_points/expected/injection_points.out 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 TestConditionLocal2
  
 (1 row)
 
+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 
+----------
+ 
+(1 row)
+
 SELECT injection_points_run('TestConditionLocal1'); -- nothing
  injection_points_run 
 ----------------------
diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/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 += {
     '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/src/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;
 
+\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', 'error');
 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
-- 
2.43.0

  signature.asc (application/pgp-signature, 832B) download