injection_points: Fix race condition with local injection point tests

michaelpq · michaelpq · commit f4083c497510 · 2024-04-09T10:31:12.000+09:00
The module relies on a shmem exit callback to clean up any injection points linked to a specific process. One of the tests checks for the case of an injection point name reused in a second connection where the first connection should clean it up, but it did not count for the fact that the shmem exit callback of the first connection may not have run when the second connection begins its work. The regress library includes a wait_pid() that can be used for this purpose, instead of a custom wait logic, so let's rely on it to wait for the first connection to exit before working with the second connection. The module gains a REGRESS_OPTS to be able to look at the regress library's dlpath. This issue could be reproduced with a hardcoded sleep() in the shmem exit callback, and the CI has been able to trigger it sporadically. Oversight in f587338. Reported-by: Bharath Rupireddy Reviewed-by: Andrey Borodin Discussion: https://postgr.es/m/ZhOd3NXAutteokGL@paquier.xyz
diff --git 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
@@ -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 
 ----------------------
@@ -193,3 +209,4 @@ SELECT injection_points_detach('TestConditionLocal1');
 (1 row)
 
 DROP EXTENSION injection_points;
+DROP FUNCTION wait_pid;
diff --git 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
@@ -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
@@ -52,3 +67,4 @@ SELECT injection_points_attach('TestConditionLocal1', 'error');
 SELECT injection_points_detach('TestConditionLocal1');
 
 DROP EXTENSION injection_points;
+DROP FUNCTION wait_pid;
