On Wed, Aug 21, 2024 at 12:46:31PM +0900, Michael Paquier wrote: > Sorry, I should have split that for clarity (one patch for the GUC, > one to change the test to use CACHED/LOAD). It is not an error > though: if we don't have a controlled way to disable the stats of the > module, then the test would fail when calling the cached callback > because we'd try to allocate some memory for the dshash entry in > pgstats. > > The second effect of initializing the shmem state of the module with > shared_preload_libraries is condition variables are set up for the > sake if the test, removing the dependency to the SQL load() call. > Both are OK, but I'd prefer introducing one use case for these two > macros in the tree, so as these can be used as a reference in the > future when developing new tests.
In short, here is a better patch set, with 0001 and 0002 introducing the pieces that the test would need to be able to use the LOAD() and CACHED() macros in 0003: - 0001: Add shmem callbacks to initialize shmem state of injection_points with shared_preload_libraries. - 0002: Add a GUC to control if the stats of the module are enabled. By default, they are disabled as they are only needed in the TAP test of injection_points for the stats. - 0003: Update the SLRU test to use INJECTION_POINT_LOAD and INJECTION_POINT_CACHED with injection_points loaded via shared_preload_libraries, removing the call to injection_points_load() in the perl test. What do you think? -- Michael
From 48a3b8b469ec52d184999d2617134ec680db378f Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Wed, 21 Aug 2024 15:12:08 +0900
Subject: [PATCH 1/3] injection_points: Add initialization of shmem state at
loading
This commits adds callbacks to initialize the shared memory state of the
module when loaded with shared_preload_libraries. This is necessary for
a upcoming change for a SLRU test, that relies on a critical section
where no allocation. Initializing the shared memory state of the module
while loading provides a control on the timing where this data is set
up.
---
.../injection_points/injection_points.c | 65 ++++++++++++++++++-
1 file changed, 62 insertions(+), 3 deletions(-)
diff --git a/src/test/modules/injection_points/injection_points.c b/src/test/modules/injection_points/injection_points.c
index 4e775c7ec6..8d83d8c401 100644
--- a/src/test/modules/injection_points/injection_points.c
+++ b/src/test/modules/injection_points/injection_points.c
@@ -68,7 +68,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 +102,13 @@ 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;
+/* 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 +121,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)
@@ -463,6 +516,12 @@ _PG_init(void)
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();
}
--
2.45.2
From fd8ab7b6845a2c56aa2c8d9c60f404f6b3407338 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Wed, 21 Aug 2024 15:16:06 +0900
Subject: [PATCH 2/3] injection_point: Add injection_points.stats
This GUC controls if statistics should be used or not in the module.
Custom statistics require the module to be loaded with
shared_preload_libraries, hence this GUC is made PGC_POSTMASTER. By
default, stats are disabled.
This will be used by an upcoming change in a test where stats should not
be used, as the test has a dependency on a critical section.
---
.../modules/injection_points/injection_points.c | 17 +++++++++++++++++
.../modules/injection_points/injection_stats.c | 8 ++++----
.../modules/injection_points/injection_stats.h | 3 +++
.../injection_points/injection_stats_fixed.c | 4 ++--
.../modules/injection_points/t/001_stats.pl | 6 ++++--
5 files changed, 30 insertions(+), 8 deletions(-)
diff --git a/src/test/modules/injection_points/injection_points.c b/src/test/modules/injection_points/injection_points.c
index 8d83d8c401..fb5eb3b586 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"
@@ -102,6 +103,9 @@ 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 = false;
+
/* Shared memory init callbacks */
static shmem_request_hook_type prev_shmem_request_hook = NULL;
static shmem_startup_hook_type prev_shmem_startup_hook = NULL;
@@ -516,6 +520,19 @@ _PG_init(void)
if (!process_shared_preload_libraries_in_progress)
return;
+ DefineCustomBoolVariable("injection_points.stats",
+ "Enables statistics for injection points.",
+ NULL,
+ &inj_stats_enabled,
+ false,
+ PGC_POSTMASTER,
+ 0,
+ NULL,
+ NULL,
+ NULL);
+
+ MarkGUCPrefixReserved("injection_points");
+
/* Shared memory initialization */
prev_shmem_request_hook = shmem_request_hook;
shmem_request_hook = injection_shmem_request;
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/injection_points/t/001_stats.pl b/src/test/modules/injection_points/t/001_stats.pl
index 0d72cd86df..9df79b5168 100644
--- a/src/test/modules/injection_points/t/001_stats.pl
+++ b/src/test/modules/injection_points/t/001_stats.pl
@@ -20,8 +20,10 @@ if ($ENV{enable_injection_points} ne 'yes')
# Node initialization
my $node = PostgreSQL::Test::Cluster->new('master');
$node->init;
-$node->append_conf('postgresql.conf',
- "shared_preload_libraries = 'injection_points'");
+$node->append_conf('postgresql.conf', qq(
+shared_preload_libraries = 'injection_points'
+injection_points.stats = true
+));
$node->start;
$node->safe_psql('postgres', 'CREATE EXTENSION injection_points;');
--
2.45.2
From e5329d080b9d8436af8f65aac118745cf1f81ca2 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Wed, 21 Aug 2024 15:09:06 +0900
Subject: [PATCH 3/3] Rework new SLRU test with injection points
Rather than the SQL injection_points_load, this commit makes the test
rely on the two macros to load and run an injection point from the
cache, acting as an example of how to use them.
---
src/backend/access/transam/multixact.c | 5 ++++-
src/test/modules/test_slru/t/001_multixact.pl | 3 +--
2 files changed, 5 insertions(+), 3 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/test_slru/t/001_multixact.pl b/src/test/modules/test_slru/t/001_multixact.pl
index f07406bf9d..882de7cd20 100644
--- a/src/test/modules/test_slru/t/001_multixact.pl
+++ b/src/test/modules/test_slru/t/001_multixact.pl
@@ -24,7 +24,7 @@ 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->start;
$node->safe_psql('postgres', q(CREATE EXTENSION injection_points));
$node->safe_psql('postgres', q(CREATE EXTENSION test_slru));
@@ -64,7 +64,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
