On Tue, Aug 20, 2024 at 02:37:34PM -0400, Alvaro Herrera wrote: > OK, I've made some minor adjustments and pushed. CI seemed OK for me, > let's see what does the BF have to say.
I see that you've gone the way with the SQL function doing a load(). Would it be worth switching the test to rely on the two macros for load and caching instead? I've mentioned that previously but never got down to present a patch for the sake of this test. This requires some more tweaks in the module to disable the stats when loaded through a GUC, and two shmem callbacks, then the test is able to work correctly. Please see attached. Thoughts? -- Michael
From db4b4601a6e930a374f6d03b622f66589d620fe2 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Wed, 21 Aug 2024 08:17:19 +0900
Subject: [PATCH] Refactor injection point test with loading
This relies on the two macros INJECTION_POINT_LOAD and
INJECTION_POINT_CACHED to trigger the test.
This adds to the test module injection_points a GUC to be able to
disable injection point stats, in case a cached point is run in a
critical section, and a code path to initialize the shmem state data of
the module when loading the module.
---
src/backend/access/transam/multixact.c | 5 +-
.../injection_points/injection_points.c | 80 ++++++++++++++++++-
.../injection_points/injection_stats.c | 8 +-
.../injection_points/injection_stats.h | 3 +
.../injection_points/injection_stats_fixed.c | 4 +-
src/test/modules/test_slru/t/001_multixact.pl | 5 +-
6 files changed, 93 insertions(+), 12 deletions(-)
diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c
index 14c2b929e2..d2d0298ac3 100644
--- a/src/backend/access/transam/multixact.c
+++ b/src/backend/access/transam/multixact.c
@@ -855,6 +855,9 @@ MultiXactIdCreateFromMembers(int nmembers, MultiXactMember *members)
}
}
+ /* Load the injection point before entering the critical section */
+ INJECTION_POINT_LOAD("multixact-create-from-members");
+
/*
* Assign the MXID and offsets range to use, and make sure there is space
* in the OFFSETs and MEMBERs files. NB: this routine does
@@ -869,7 +872,7 @@ MultiXactIdCreateFromMembers(int nmembers, MultiXactMember *members)
*/
multi = GetNewMultiXactId(nmembers, &offset);
- INJECTION_POINT("multixact-create-from-members");
+ INJECTION_POINT_CACHED("multixact-create-from-members");
/* Make an XLOG entry describing the new MXID. */
xlrec.mid = multi;
diff --git a/src/test/modules/injection_points/injection_points.c b/src/test/modules/injection_points/injection_points.c
index 4e775c7ec6..8b14f5fc19 100644
--- a/src/test/modules/injection_points/injection_points.c
+++ b/src/test/modules/injection_points/injection_points.c
@@ -28,6 +28,7 @@
#include "storage/lwlock.h"
#include "storage/shmem.h"
#include "utils/builtins.h"
+#include "utils/guc.h"
#include "utils/injection_point.h"
#include "utils/memutils.h"
#include "utils/wait_event.h"
@@ -68,7 +69,12 @@ typedef struct InjectionPointCondition
*/
static List *inj_list_local = NIL;
-/* Shared state information for injection points. */
+/*
+ * Shared state information for injection points.
+ *
+ * This state data can be initialized in two ways: dynamically with a DSM
+ * or when loading the module.
+ */
typedef struct InjectionPointSharedState
{
/* Protects access to other fields */
@@ -97,8 +103,16 @@ 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;
+/* GUC variable */
+bool inj_stats_enabled = true;
+
+/* Shared memory init callbacks */
+static shmem_request_hook_type prev_shmem_request_hook = NULL;
+static shmem_startup_hook_type prev_shmem_startup_hook = NULL;
+
/*
- * Callback for shared memory area initialization.
+ * Routine for shared memory area initialization, used as a callback
+ * when initializing dynamically with a DSM or when loading the module.
*/
static void
injection_point_init_state(void *ptr)
@@ -111,8 +125,51 @@ injection_point_init_state(void *ptr)
ConditionVariableInit(&state->wait_point);
}
+/* Shared memory initialization when loading module */
+static void
+injection_shmem_request(void)
+{
+ Size size;
+
+ if (prev_shmem_request_hook)
+ prev_shmem_request_hook();
+
+ size = MAXALIGN(sizeof(InjectionPointSharedState));
+ RequestAddinShmemSpace(size);
+}
+
+static void
+injection_shmem_startup(void)
+{
+ bool found;
+
+ if (prev_shmem_startup_hook)
+ prev_shmem_startup_hook();
+
+ /* reset in case this is a restart within the postmaster */
+ inj_state = NULL;
+
+ /* Create or attach to the shared memory state */
+ LWLockAcquire(AddinShmemInitLock, LW_EXCLUSIVE);
+
+ inj_state = ShmemInitStruct("injection_points",
+ sizeof(InjectionPointSharedState),
+ &found);
+
+ if (!found)
+ {
+ /*
+ * First time through, so initialize. This is shared with the
+ * dynamic initialization using a DSM.
+ */
+ injection_point_init_state(inj_state);
+ }
+
+ LWLockRelease(AddinShmemInitLock);
+}
+
/*
- * Initialize shared memory area for this module.
+ * Initialize shared memory area for this module through DSM.
*/
static void
injection_init_shmem(void)
@@ -460,9 +517,26 @@ injection_points_detach(PG_FUNCTION_ARGS)
void
_PG_init(void)
{
+ DefineCustomBoolVariable("injection_points.stats",
+ "Enables statistics for injection points.",
+ NULL,
+ &inj_stats_enabled,
+ true,
+ PGC_SIGHUP,
+ 0,
+ NULL,
+ NULL,
+ NULL);
+
if (!process_shared_preload_libraries_in_progress)
return;
+ /* Shared memory initialization */
+ prev_shmem_request_hook = shmem_request_hook;
+ shmem_request_hook = injection_shmem_request;
+ prev_shmem_startup_hook = shmem_startup_hook;
+ shmem_startup_hook = injection_shmem_startup;
+
pgstat_register_inj();
pgstat_register_inj_fixed();
}
diff --git a/src/test/modules/injection_points/injection_stats.c b/src/test/modules/injection_points/injection_stats.c
index 78042074ff..582686a0a8 100644
--- a/src/test/modules/injection_points/injection_stats.c
+++ b/src/test/modules/injection_points/injection_stats.c
@@ -91,7 +91,7 @@ pgstat_fetch_stat_injentry(const char *name)
{
PgStat_StatInjEntry *entry = NULL;
- if (!inj_stats_loaded)
+ if (!inj_stats_loaded || !inj_stats_enabled)
return NULL;
/* Compile the lookup key as a hash of the point name */
@@ -123,7 +123,7 @@ pgstat_create_inj(const char *name)
PgStatShared_InjectionPoint *shstatent;
/* leave if disabled */
- if (!inj_stats_loaded)
+ if (!inj_stats_loaded || !inj_stats_enabled)
return;
entry_ref = pgstat_get_entry_ref_locked(PGSTAT_KIND_INJECTION, InvalidOid,
@@ -142,7 +142,7 @@ void
pgstat_drop_inj(const char *name)
{
/* leave if disabled */
- if (!inj_stats_loaded)
+ if (!inj_stats_loaded || !inj_stats_enabled)
return;
if (!pgstat_drop_entry(PGSTAT_KIND_INJECTION, InvalidOid,
@@ -164,7 +164,7 @@ pgstat_report_inj(const char *name)
PgStat_StatInjEntry *statent;
/* leave if disabled */
- if (!inj_stats_loaded)
+ if (!inj_stats_loaded || !inj_stats_enabled)
return;
entry_ref = pgstat_get_entry_ref_locked(PGSTAT_KIND_INJECTION, InvalidOid,
diff --git a/src/test/modules/injection_points/injection_stats.h b/src/test/modules/injection_points/injection_stats.h
index 126c110169..c48d533b4b 100644
--- a/src/test/modules/injection_points/injection_stats.h
+++ b/src/test/modules/injection_points/injection_stats.h
@@ -15,6 +15,9 @@
#ifndef INJECTION_STATS
#define INJECTION_STATS
+/* GUC variable */
+extern bool inj_stats_enabled;
+
/* injection_stats.c */
extern void pgstat_register_inj(void);
extern void pgstat_create_inj(const char *name);
diff --git a/src/test/modules/injection_points/injection_stats_fixed.c b/src/test/modules/injection_points/injection_stats_fixed.c
index 82b07e5332..2fed178b7a 100644
--- a/src/test/modules/injection_points/injection_stats_fixed.c
+++ b/src/test/modules/injection_points/injection_stats_fixed.c
@@ -146,7 +146,7 @@ pgstat_report_inj_fixed(uint32 numattach,
PgStatShared_InjectionPointFixed *stats_shmem;
/* leave if disabled */
- if (!inj_fixed_loaded)
+ if (!inj_fixed_loaded || !inj_stats_enabled)
return;
stats_shmem = pgstat_get_custom_shmem_data(PGSTAT_KIND_INJECTION_FIXED);
@@ -172,7 +172,7 @@ injection_points_stats_fixed(PG_FUNCTION_ARGS)
bool nulls[5] = {0};
PgStat_StatInjFixedEntry *stats;
- if (!inj_fixed_loaded)
+ if (!inj_fixed_loaded || !inj_stats_enabled)
PG_RETURN_NULL();
pgstat_snapshot_fixed(PGSTAT_KIND_INJECTION_FIXED);
diff --git a/src/test/modules/test_slru/t/001_multixact.pl b/src/test/modules/test_slru/t/001_multixact.pl
index f07406bf9d..793706c5f2 100644
--- a/src/test/modules/test_slru/t/001_multixact.pl
+++ b/src/test/modules/test_slru/t/001_multixact.pl
@@ -24,7 +24,9 @@ my ($node, $result);
$node = PostgreSQL::Test::Cluster->new('mike');
$node->init;
$node->append_conf('postgresql.conf',
- "shared_preload_libraries = 'test_slru'");
+ "shared_preload_libraries = 'test_slru,injection_points'");
+$node->append_conf('postgresql.conf',
+ "injection_points.stats = false");
$node->start;
$node->safe_psql('postgres', q(CREATE EXTENSION injection_points));
$node->safe_psql('postgres', q(CREATE EXTENSION test_slru));
@@ -64,7 +66,6 @@ $node->safe_psql('postgres',
$creator->query_until(
qr/start/, q{
\echo start
- SELECT injection_points_load('multixact-create-from-members');
SELECT test_create_multixact();
});
--
2.45.2
signature.asc
Description: PGP signature
