On Sat, Mar 16, 2024 at 08:40:21AM +0900, Michael Paquier wrote: > Linking all the points to a PID with a injection_points_attach_local() > that switches a static flag while registering a before_shmem_exit() to > do an automated cleanup sounds like the simplest approach to me based > on what I'm reading on this thread.
Please find a patch to do exactly that, without touching the backend APIs. 0001 adds a new function call injection_points_local() that can be added on top of a SQL test to make it concurrent-safe. 0002 is the fix for the GIN tests. I am going to add an open item to not forget about all that. Comments are welcome. -- Michael
From a4c219f0142a36b2a009af1f9e9affdeab6ab383 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Mon, 18 Mar 2024 09:53:05 +0900
Subject: [PATCH v2 1/2] Introduce runtime conditions in module
injection_points
This adds a new SQL function injection_points_local() that can be used
to force injection points to be run only on the process where they are
attached. This is handy for SQL tests to:
- detach automatically local injection points when the process exits.
- allow tests with injection points to run concurrently with other test
suites.
---
.../expected/injection_points.out | 77 ++++++++
.../injection_points--1.0.sql | 11 ++
.../injection_points/injection_points.c | 176 ++++++++++++++++++
.../injection_points/sql/injection_points.sql | 20 ++
src/tools/pgindent/typedefs.list | 1 +
5 files changed, 285 insertions(+)
diff --git a/src/test/modules/injection_points/expected/injection_points.out b/src/test/modules/injection_points/expected/injection_points.out
index 827456ccc5..f24a602def 100644
--- a/src/test/modules/injection_points/expected/injection_points.out
+++ b/src/test/modules/injection_points/expected/injection_points.out
@@ -115,4 +115,81 @@ NOTICE: notice triggered for injection point TestInjectionLog2
(1 row)
+SELECT injection_points_detach('TestInjectionLog2');
+ injection_points_detach
+-------------------------
+
+(1 row)
+
+-- Runtime conditions
+SELECT injection_points_attach('TestConditionError', 'error');
+ injection_points_attach
+-------------------------
+
+(1 row)
+
+-- Any follow-up injection point attached will be local to this process.
+SELECT injection_points_local();
+ injection_points_local
+------------------------
+
+(1 row)
+
+SELECT injection_points_attach('TestConditionLocal1', 'error');
+ injection_points_attach
+-------------------------
+
+(1 row)
+
+SELECT injection_points_attach('TestConditionLocal2', 'notice');
+ injection_points_attach
+-------------------------
+
+(1 row)
+
+SELECT injection_points_run('TestConditionLocal1'); -- error
+ERROR: error triggered for injection point TestConditionLocal1
+SELECT injection_points_run('TestConditionLocal2'); -- notice
+NOTICE: notice triggered for injection point TestConditionLocal2
+ injection_points_run
+----------------------
+
+(1 row)
+
+-- reload, local injection points should be gone.
+\c
+SELECT injection_points_run('TestConditionLocal1'); -- nothing
+ injection_points_run
+----------------------
+
+(1 row)
+
+SELECT injection_points_run('TestConditionLocal2'); -- nothing
+ injection_points_run
+----------------------
+
+(1 row)
+
+SELECT injection_points_run('TestConditionError'); -- error
+ERROR: error triggered for injection point TestConditionError
+SELECT injection_points_detach('TestConditionError');
+ injection_points_detach
+-------------------------
+
+(1 row)
+
+-- Attaching injection points that use the same name as one defined locally
+-- previously should work.
+SELECT injection_points_attach('TestConditionLocal1', 'error');
+ injection_points_attach
+-------------------------
+
+(1 row)
+
+SELECT injection_points_detach('TestConditionLocal1');
+ injection_points_detach
+-------------------------
+
+(1 row)
+
DROP EXTENSION injection_points;
diff --git a/src/test/modules/injection_points/injection_points--1.0.sql b/src/test/modules/injection_points/injection_points--1.0.sql
index 0a2e59aba7..1813a4d6d8 100644
--- a/src/test/modules/injection_points/injection_points--1.0.sql
+++ b/src/test/modules/injection_points/injection_points--1.0.sql
@@ -34,6 +34,17 @@ RETURNS void
AS 'MODULE_PATHNAME', 'injection_points_wakeup'
LANGUAGE C STRICT PARALLEL UNSAFE;
+--
+-- injection_points_local()
+--
+-- Trigger switch to link any future injection points attached to the
+-- current process, useful to make SQL tests concurrently-safe.
+--
+CREATE FUNCTION injection_points_local()
+RETURNS void
+AS 'MODULE_PATHNAME', 'injection_points_local'
+LANGUAGE C STRICT PARALLEL UNSAFE;
+
--
-- injection_points_detach()
--
diff --git a/src/test/modules/injection_points/injection_points.c b/src/test/modules/injection_points/injection_points.c
index 0730254d54..8be081db99 100644
--- a/src/test/modules/injection_points/injection_points.c
+++ b/src/test/modules/injection_points/injection_points.c
@@ -18,8 +18,10 @@
#include "postgres.h"
#include "fmgr.h"
+#include "miscadmin.h"
#include "storage/condition_variable.h"
#include "storage/dsm_registry.h"
+#include "storage/ipc.h"
#include "storage/lwlock.h"
#include "storage/shmem.h"
#include "utils/builtins.h"
@@ -31,6 +33,23 @@ PG_MODULE_MAGIC;
/* Maximum number of waits usable in injection points at once */
#define INJ_MAX_WAIT 8
#define INJ_NAME_MAXLEN 64
+#define INJ_MAX_CONDITION 4
+
+/*
+ * Conditions related to injection points. This tracks in shared memory the
+ * runtime conditions under which an injection point is allowed to run.
+ *
+ * If more types of runtime conditions need to be tracked, this structure
+ * should be expanded.
+ */
+typedef struct InjectionPointCondition
+{
+ /* Name of the injection point related to its */
+ char name[INJ_NAME_MAXLEN];
+
+ /* ID of the process where the injection point is allowed to run */
+ int pid;
+} InjectionPointCondition;
/* Shared state information for injection points. */
typedef struct InjectionPointSharedState
@@ -44,6 +63,9 @@ typedef struct InjectionPointSharedState
/* Names of injection points attached to wait counters */
char name[INJ_MAX_WAIT][INJ_NAME_MAXLEN];
+ /* Condition to run an injection point */
+ InjectionPointCondition condition[INJ_MAX_CONDITION];
+
/* Condition variable used for waits and wakeups */
ConditionVariable wait_point;
} InjectionPointSharedState;
@@ -55,6 +77,8 @@ extern PGDLLEXPORT void injection_error(const char *name);
extern PGDLLEXPORT void injection_notice(const char *name);
extern PGDLLEXPORT void injection_wait(const char *name);
+/* track if injection points attached in this process are linked to it */
+static bool injection_point_local = false;
/*
* Callback for shared memory area initialization.
@@ -67,6 +91,7 @@ injection_point_init_state(void *ptr)
SpinLockInit(&state->lock);
memset(state->wait_counts, 0, sizeof(state->wait_counts));
memset(state->name, 0, sizeof(state->name));
+ memset(state->condition, 0, sizeof(state->condition));
ConditionVariableInit(&state->wait_point);
}
@@ -87,16 +112,92 @@ injection_init_shmem(void)
&found);
}
+/*
+ * Check runtime conditions associated to an injection point.
+ *
+ * Returns true if the named injection point is allowed to run, and false
+ * otherwise. Multiple conditions can be associated to a single injection
+ * point, so check them all.
+ */
+static bool
+injection_point_allowed(const char *name)
+{
+ bool result = true;
+
+ if (inj_state == NULL)
+ injection_init_shmem();
+
+ SpinLockAcquire(&inj_state->lock);
+
+ for (int i = 0; i < INJ_MAX_CONDITION; i++)
+ {
+ InjectionPointCondition *condition = &inj_state->condition[i];
+
+ if (strcmp(condition->name, name) == 0)
+ {
+ /*
+ * Check if this injection point is allowed to run in this
+ * process.
+ */
+ if (MyProcPid != condition->pid)
+ {
+ result = false;
+ break;
+ }
+ }
+ }
+
+ SpinLockRelease(&inj_state->lock);
+
+ return result;
+}
+
+/*
+ * before_shmem_exit callback to remove injection points linked to a
+ * specific process.
+ */
+static void
+injection_points_cleanup(int code, Datum arg)
+{
+ /* Leave if nothing is tracked locally */
+ if (!injection_point_local)
+ return;
+
+ SpinLockAcquire(&inj_state->lock);
+ for (int i = 0; i < INJ_MAX_CONDITION; i++)
+ {
+ InjectionPointCondition *condition = &inj_state->condition[i];
+
+ if (condition->name[0] == '\0')
+ continue;
+
+ if (condition->pid != MyProcPid)
+ continue;
+
+ /* Detach the injection point and unregister condition */
+ InjectionPointDetach(condition->name);
+ condition->name[0] = '\0';
+ condition->pid = 0;
+ }
+ SpinLockRelease(&inj_state->lock);
+}
+
/* Set of callbacks available to be attached to an injection point. */
void
injection_error(const char *name)
{
+ if (!injection_point_allowed(name))
+ return;
+
elog(ERROR, "error triggered for injection point %s", name);
}
void
injection_notice(const char *name)
{
+ if (!injection_point_allowed(name))
+ return;
+
elog(NOTICE, "notice triggered for injection point %s", name);
}
@@ -111,6 +212,9 @@ injection_wait(const char *name)
if (inj_state == NULL)
injection_init_shmem();
+ if (!injection_point_allowed(name))
+ return;
+
/*
* Use the injection point name for this custom wait event. Note that
* this custom wait event name is not released, but we don't care much for
@@ -182,6 +286,35 @@ injection_points_attach(PG_FUNCTION_ARGS)
InjectionPointAttach(name, "injection_points", function);
+ if (injection_point_local)
+ {
+ int index = -1;
+
+ /*
+ * Register runtime condition to link this injection point to the
+ * current process.
+ */
+ SpinLockAcquire(&inj_state->lock);
+ for (int i = 0; i < INJ_MAX_CONDITION; i++)
+ {
+ InjectionPointCondition *condition = &inj_state->condition[i];
+
+ if (condition->name[0] == '\0')
+ {
+ index = i;
+ strlcpy(condition->name, name, INJ_NAME_MAXLEN);
+ condition->pid = MyProcPid;
+ break;
+ }
+ }
+ SpinLockRelease(&inj_state->lock);
+
+ if (index < 0)
+ elog(FATAL,
+ "could not find free slot for condition of injection point %s",
+ name);
+ }
+
PG_RETURN_VOID();
}
@@ -235,6 +368,32 @@ injection_points_wakeup(PG_FUNCTION_ARGS)
PG_RETURN_VOID();
}
+/*
+ * injection_points_local
+ *
+ * Track if any injection point created in this process ought to run only
+ * in this process. Such injection points are detached automatically when
+ * this process exits. This is useful to make test suites concurrent-safe.
+ */
+PG_FUNCTION_INFO_V1(injection_points_local);
+Datum
+injection_points_local(PG_FUNCTION_ARGS)
+{
+ /* Enable flag to add a runtime condition based on this process ID */
+ injection_point_local = true;
+
+ if (inj_state == NULL)
+ injection_init_shmem();
+
+ /*
+ * Register a before_shmem_exit callback to remove any injection points
+ * linked to this process.
+ */
+ before_shmem_exit(injection_points_cleanup, (Datum) 0);
+
+ PG_RETURN_VOID();
+}
+
/*
* SQL function for dropping an injection point.
*/
@@ -246,5 +405,22 @@ injection_points_detach(PG_FUNCTION_ARGS)
InjectionPointDetach(name);
+ if (inj_state == NULL)
+ injection_init_shmem();
+
+ /* Clean up any conditions associated to this injection point */
+ SpinLockAcquire(&inj_state->lock);
+ for (int i = 0; i < INJ_MAX_CONDITION; i++)
+ {
+ InjectionPointCondition *condition = &inj_state->condition[i];
+
+ if (strcmp(condition->name, name) == 0)
+ {
+ condition->pid = 0;
+ condition->name[0] = '\0';
+ }
+ }
+ SpinLockRelease(&inj_state->lock);
+
PG_RETURN_VOID();
}
diff --git a/src/test/modules/injection_points/sql/injection_points.sql b/src/test/modules/injection_points/sql/injection_points.sql
index 23c7e435ad..27424bdfe7 100644
--- a/src/test/modules/injection_points/sql/injection_points.sql
+++ b/src/test/modules/injection_points/sql/injection_points.sql
@@ -30,5 +30,25 @@ SELECT injection_points_run('TestInjectionLog2'); -- notice
SELECT injection_points_detach('TestInjectionLog'); -- fails
SELECT injection_points_run('TestInjectionLog2'); -- notice
+SELECT injection_points_detach('TestInjectionLog2');
+
+-- Runtime conditions
+SELECT injection_points_attach('TestConditionError', 'error');
+-- Any follow-up injection point attached will be local to this process.
+SELECT injection_points_local();
+SELECT injection_points_attach('TestConditionLocal1', 'error');
+SELECT injection_points_attach('TestConditionLocal2', 'notice');
+SELECT injection_points_run('TestConditionLocal1'); -- error
+SELECT injection_points_run('TestConditionLocal2'); -- notice
+-- reload, local injection points should be gone.
+\c
+SELECT injection_points_run('TestConditionLocal1'); -- nothing
+SELECT injection_points_run('TestConditionLocal2'); -- nothing
+SELECT injection_points_run('TestConditionError'); -- error
+SELECT injection_points_detach('TestConditionError');
+-- Attaching injection points that use the same name as one defined locally
+-- previously should work.
+SELECT injection_points_attach('TestConditionLocal1', 'error');
+SELECT injection_points_detach('TestConditionLocal1');
DROP EXTENSION injection_points;
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 6ca93b1e47..d429d6035a 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -1210,6 +1210,7 @@ InitSampleScan_function
InitializeDSMForeignScan_function
InitializeWorkerForeignScan_function
InjectionPointCacheEntry
+InjectionPointCondition
InjectionPointEntry
InjectionPointSharedState
InlineCodeBlock
--
2.43.0
From 8d7338f68b6add2b28acfebd7541ef5c21913264 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Mon, 18 Mar 2024 09:56:35 +0900
Subject: [PATCH v2 2/2] Switch GIN tests with injection points to be
concurrent-safe
---
src/test/modules/gin/Makefile | 3 ---
src/test/modules/gin/expected/gin_incomplete_splits.out | 7 +++++++
src/test/modules/gin/meson.build | 2 --
src/test/modules/gin/sql/gin_incomplete_splits.sql | 3 +++
4 files changed, 10 insertions(+), 5 deletions(-)
diff --git a/src/test/modules/gin/Makefile b/src/test/modules/gin/Makefile
index da4c9cea5e..e007e38ac2 100644
--- a/src/test/modules/gin/Makefile
+++ b/src/test/modules/gin/Makefile
@@ -4,9 +4,6 @@ EXTRA_INSTALL = src/test/modules/injection_points
REGRESS = gin_incomplete_splits
-# The injection points are cluster-wide, so disable installcheck
-NO_INSTALLCHECK = 1
-
ifdef USE_PGXS
PG_CONFIG = pg_config
PGXS := $(shell $(PG_CONFIG) --pgxs)
diff --git a/src/test/modules/gin/expected/gin_incomplete_splits.out b/src/test/modules/gin/expected/gin_incomplete_splits.out
index 822b78cffc..f503d3dd3c 100644
--- a/src/test/modules/gin/expected/gin_incomplete_splits.out
+++ b/src/test/modules/gin/expected/gin_incomplete_splits.out
@@ -12,6 +12,13 @@
-- This uses injection points to cause errors that leave some page
-- splits in "incomplete" state
create extension injection_points;
+-- Make all injection points local to this process, for concurrency.
+SELECT injection_points_local();
+ injection_points_local
+------------------------
+
+(1 row)
+
-- Use the index for all the queries
set enable_seqscan=off;
-- Print a NOTICE whenever an incomplete split gets fixed
diff --git a/src/test/modules/gin/meson.build b/src/test/modules/gin/meson.build
index 5ec0760a27..9734b51de2 100644
--- a/src/test/modules/gin/meson.build
+++ b/src/test/modules/gin/meson.build
@@ -12,7 +12,5 @@ tests += {
'sql': [
'gin_incomplete_splits',
],
- # The injection points are cluster-wide, so disable installcheck
- 'runningcheck': false,
},
}
diff --git a/src/test/modules/gin/sql/gin_incomplete_splits.sql b/src/test/modules/gin/sql/gin_incomplete_splits.sql
index 71253b71f3..f413b54708 100644
--- a/src/test/modules/gin/sql/gin_incomplete_splits.sql
+++ b/src/test/modules/gin/sql/gin_incomplete_splits.sql
@@ -14,6 +14,9 @@
-- splits in "incomplete" state
create extension injection_points;
+-- Make all injection points local to this process, for concurrency.
+SELECT injection_points_local();
+
-- Use the index for all the queries
set enable_seqscan=off;
--
2.43.0
signature.asc
Description: PGP signature
