On Thu, Mar 14, 2024 at 07:13:53PM -0400, Tom Lane wrote: > Michael Paquier <mich...@paquier.xyz> writes: >> Or we could just disable runningcheck because of the concurrency >> requirement in this test. The test would still be able to run, just >> less times. > > No, actually we *must* mark all these tests NO_INSTALLCHECK if we > stick with the current definition of injection points. The point > of installcheck mode is that the tests are supposed to be safe to > run in a live installation. Side-effects occurring in other > databases are completely not OK.
I really don't want to plug any runtime conditions into the backend APIs, because there can be so much more that can be done there than only restricting a callback to a database. I can imagine process type restrictions, process PID restrictions, etc. So this knowledge should stick into the test module itself, and be expanded there. That's easier ABI-wise, as well. > I can see that some tests would want to be able to inject code > cluster-wide, but I bet that's going to be a small minority. > I suggest that we invent a notion of "global" vs "local" > injection points, where a "local" one only fires in the DB it > was defined in. Then only tests that require a global injection > point need to be NO_INSTALLCHECK. Attached is a POC of what could be done. I have extended the module injection_points so as it is possible to register what I'm calling a "condition" in the module that can be defined with a new SQL function. The condition is stored in shared memory with the point name, then at runtime the conditions are cross-checked in the callbacks. With the interface of this patch, the condition should be registered *before* a point is attached, but this stuff could also be written so as injection_points_attach() takes an optional argument with a database name. Or this could use a different, new SQL function, say a injection_points_attach_local() that registers a condition with MyDatabaseId on top of attaching the point, making the whole happening while holding once the spinlock of the shmem state for the module. By the way, modules/gin/ was missing missing a detach, so the test was not repeatable with successive installchecks. Adding a pg_sleep of a few seconds after 'gin-leave-leaf-split-incomplete' is registered enlarges the window, and the patch avoids failures when running installcheck in parallel for modules/gin/ and something else using gin, like contrib/btree_gin/: while make USE_MODULE_DB=1 installcheck; do :; done 0001 is the condition facility for the module, 0002 is a fix for the GIN test. Thoughts are welcome. -- Michael
From 5f00092a0bd193f70d9daff77caacfc2ba83ca52 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Fri, 15 Mar 2024 16:03:05 +0900
Subject: [PATCH 1/2] Introduce runtime conditions in module injection_points
This adds a new SQL function injection_points_condition() that can be
used to register a runtime condition to an injection point, declared or
not. For now, it is possible to register a database name to make an
injection point run only on a defined database.
---
.../expected/injection_points.out | 54 ++++++++
.../injection_points--1.0.sql | 13 ++
.../injection_points/injection_points.c | 131 ++++++++++++++++++
.../injection_points/sql/injection_points.sql | 14 ++
4 files changed, 212 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..ccb83b291f 100644
--- a/src/test/modules/injection_points/expected/injection_points.out
+++ b/src/test/modules/injection_points/expected/injection_points.out
@@ -115,4 +115,58 @@ NOTICE: notice triggered for injection point TestInjectionLog2
(1 row)
+SELECT injection_points_detach('TestInjectionLog2');
+ injection_points_detach
+-------------------------
+
+(1 row)
+
+-- Conditions
+SELECT current_database() AS datname \gset
+-- Register injection point to run on database 'postgres'.
+SELECT injection_points_attach('TestConditionError', 'error');
+ injection_points_attach
+-------------------------
+
+(1 row)
+
+SELECT injection_points_condition('TestConditionError', 'postgres');
+ injection_points_condition
+----------------------------
+
+(1 row)
+
+SELECT injection_points_run('TestConditionError'); -- nothing
+ injection_points_run
+----------------------
+
+(1 row)
+
+SELECT injection_points_detach('TestConditionError');
+ injection_points_detach
+-------------------------
+
+(1 row)
+
+-- Register injection point to run on current database
+SELECT injection_points_attach('TestConditionError', 'error');
+ injection_points_attach
+-------------------------
+
+(1 row)
+
+SELECT injection_points_condition('TestConditionError', :'datname');
+ injection_points_condition
+----------------------------
+
+(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)
+
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..b707f23dad 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,19 @@ RETURNS void
AS 'MODULE_PATHNAME', 'injection_points_wakeup'
LANGUAGE C STRICT PARALLEL UNSAFE;
+--
+-- injection_points_condition()
+--
+-- Register a runtime condition associated to an injection point.
+--
+-- dbname can be specified to restrict an injection point to run on a
+-- wanted database.
+--
+CREATE FUNCTION injection_points_condition(IN point_name TEXT, IN dbname text)
+RETURNS void
+AS 'MODULE_PATHNAME', 'injection_points_condition'
+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..ce9317f0ee 100644
--- a/src/test/modules/injection_points/injection_points.c
+++ b/src/test/modules/injection_points/injection_points.c
@@ -17,7 +17,9 @@
#include "postgres.h"
+#include "commands/dbcommands.h"
#include "fmgr.h"
+#include "miscadmin.h"
#include "storage/condition_variable.h"
#include "storage/dsm_registry.h"
#include "storage/lwlock.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 8
+
+/*
+ * Conditions related to injection points. This tracks in shared memory the
+ * some 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];
+
+ /* Oid of the database where the injection point is allowed to run */
+ Oid dboid;
+} 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;
@@ -67,6 +89,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 +110,62 @@ 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 that if the database matches with what's used in this
+ * backend.
+ */
+ if (MyDatabaseId != condition->dboid)
+ {
+ result = false;
+ break;
+ }
+ }
+ }
+
+ SpinLockRelease(&inj_state->lock);
+
+ return result;
+}
+
/* 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 +180,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
@@ -235,6 +307,48 @@ injection_points_wakeup(PG_FUNCTION_ARGS)
PG_RETURN_VOID();
}
+/*
+ * injection_points_condition
+ *
+ * Register a runtime condition associated to an injection point. Currently,
+ * it is possible to define a database name where an injection point can be
+ * restricted to run on.
+ */
+PG_FUNCTION_INFO_V1(injection_points_condition);
+Datum
+injection_points_condition(PG_FUNCTION_ARGS)
+{
+ char *name = text_to_cstring(PG_GETARG_TEXT_PP(0));
+ char *dbname = text_to_cstring(PG_GETARG_TEXT_PP(1));
+ Oid dboid = get_database_oid(dbname, false);
+ int index = -1;
+
+ 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 (condition->name[0] == '\0')
+ {
+ index = i;
+ strlcpy(condition->name, name, INJ_NAME_MAXLEN);
+ condition->dboid = dboid;
+ break;
+ }
+ }
+ SpinLockRelease(&inj_state->lock);
+
+ if (index < 0)
+ elog(ERROR, "could not find free slot for condition of injection point %s",
+ name);
+
+ PG_RETURN_VOID();
+}
+
/*
* SQL function for dropping an injection point.
*/
@@ -246,5 +360,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->dboid = InvalidOid;
+ 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..1b52e7e6d7 100644
--- a/src/test/modules/injection_points/sql/injection_points.sql
+++ b/src/test/modules/injection_points/sql/injection_points.sql
@@ -30,5 +30,19 @@ SELECT injection_points_run('TestInjectionLog2'); -- notice
SELECT injection_points_detach('TestInjectionLog'); -- fails
SELECT injection_points_run('TestInjectionLog2'); -- notice
+SELECT injection_points_detach('TestInjectionLog2');
+
+-- Conditions
+SELECT current_database() AS datname \gset
+-- Register injection point to run on database 'postgres'.
+SELECT injection_points_attach('TestConditionError', 'error');
+SELECT injection_points_condition('TestConditionError', 'postgres');
+SELECT injection_points_run('TestConditionError'); -- nothing
+SELECT injection_points_detach('TestConditionError');
+-- Register injection point to run on current database
+SELECT injection_points_attach('TestConditionError', 'error');
+SELECT injection_points_condition('TestConditionError', :'datname');
+SELECT injection_points_run('TestConditionError'); -- error
+SELECT injection_points_detach('TestConditionError');
DROP EXTENSION injection_points;
--
2.43.0
From f0b6e81a8652cbfe8733c34bcd035997392011b8 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Fri, 15 Mar 2024 16:26:52 +0900
Subject: [PATCH 2/2] Restrict GIN test with injection points to run on local
database
It is possible that this test conflicts with parallel installcheck
activities. While on it, add a forgotten detach to the GIN test to make
it repeatable.
---
.../gin/expected/gin_incomplete_splits.out | 27 +++++++++++++++++++
.../modules/gin/sql/gin_incomplete_splits.sql | 8 ++++++
2 files changed, 35 insertions(+)
diff --git a/src/test/modules/gin/expected/gin_incomplete_splits.out b/src/test/modules/gin/expected/gin_incomplete_splits.out
index 9a60f8c128..7803dd7a90 100644
--- a/src/test/modules/gin/expected/gin_incomplete_splits.out
+++ b/src/test/modules/gin/expected/gin_incomplete_splits.out
@@ -14,7 +14,16 @@
create extension injection_points;
-- Use the index for all the queries
set enable_seqscan=off;
+-- Limit all the injection points of this script to run on the current
+-- database, for safety under concurrent activity.
+SELECT current_database() AS datname \gset
-- Print a NOTICE whenever an incomplete split gets fixed
+SELECT injection_points_condition('gin-finish-incomplete-split', :'datname');
+ injection_points_condition
+----------------------------
+
+(1 row)
+
SELECT injection_points_attach('gin-finish-incomplete-split', 'notice');
injection_points_attach
-------------------------
@@ -109,6 +118,12 @@ select verify(:next_i);
--
-- Test incomplete leaf split
--
+SELECT injection_points_condition('gin-leave-leaf-split-incomplete', :'datname');
+ injection_points_condition
+----------------------------
+
+(1 row)
+
SELECT injection_points_attach('gin-leave-leaf-split-incomplete', 'error');
injection_points_attach
-------------------------
@@ -145,6 +160,12 @@ select verify(:next_i);
--
-- Test incomplete internal page split
--
+SELECT injection_points_condition('gin-leave-internal-split-incomplete', :'datname');
+ injection_points_condition
+----------------------------
+
+(1 row)
+
SELECT injection_points_attach('gin-leave-internal-split-incomplete', 'error');
injection_points_attach
-------------------------
@@ -178,3 +199,9 @@ select verify(:next_i);
t
(1 row)
+SELECT injection_points_detach('gin-finish-incomplete-split');
+ injection_points_detach
+-------------------------
+
+(1 row)
+
diff --git a/src/test/modules/gin/sql/gin_incomplete_splits.sql b/src/test/modules/gin/sql/gin_incomplete_splits.sql
index 4e180b2519..5b4521edd9 100644
--- a/src/test/modules/gin/sql/gin_incomplete_splits.sql
+++ b/src/test/modules/gin/sql/gin_incomplete_splits.sql
@@ -17,7 +17,12 @@ create extension injection_points;
-- Use the index for all the queries
set enable_seqscan=off;
+-- Limit all the injection points of this script to run on the current
+-- database, for safety under concurrent activity.
+SELECT current_database() AS datname \gset
+
-- Print a NOTICE whenever an incomplete split gets fixed
+SELECT injection_points_condition('gin-finish-incomplete-split', :'datname');
SELECT injection_points_attach('gin-finish-incomplete-split', 'notice');
--
@@ -111,6 +116,7 @@ select verify(:next_i);
--
-- Test incomplete leaf split
--
+SELECT injection_points_condition('gin-leave-leaf-split-incomplete', :'datname');
SELECT injection_points_attach('gin-leave-leaf-split-incomplete', 'error');
select insert_until_fail(:next_i) as next_i
\gset
@@ -129,6 +135,7 @@ select verify(:next_i);
--
-- Test incomplete internal page split
--
+SELECT injection_points_condition('gin-leave-internal-split-incomplete', :'datname');
SELECT injection_points_attach('gin-leave-internal-split-incomplete', 'error');
select insert_until_fail(:next_i, 100) as next_i
\gset
@@ -142,3 +149,4 @@ select insert_n(:next_i, 10) as next_i
\gset
-- Verify that a scan still works
select verify(:next_i);
+SELECT injection_points_detach('gin-finish-incomplete-split');
--
2.43.0
signature.asc
Description: PGP signature
