From f4083c49751018ae2140de8bf752b8d60485a6ca Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Tue, 9 Apr 2024 10:31:12 +0900 Subject: [PATCH] injection_points: Fix race condition with local injection point tests 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 f587338dec87. Reported-by: Bharath Rupireddy Reviewed-by: Andrey Borodin Discussion: https://postgr.es/m/ZhOd3NXAutteokGL@paquier.xyz --- src/test/modules/injection_points/Makefile | 1 + .../expected/injection_points.out | 17 +++++++++++++++++ src/test/modules/injection_points/meson.build | 1 + .../injection_points/sql/injection_points.sql | 16 ++++++++++++++++ 4 files changed, 35 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..1341d66c92 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 ---------------------- @@ -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 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..71e2972a7e 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 @@ -52,3 +67,4 @@ SELECT injection_points_attach('TestConditionLocal1', 'error'); SELECT injection_points_detach('TestConditionLocal1'); DROP EXTENSION injection_points; +DROP FUNCTION wait_pid;