Hi all, (Andres in CC.) While reading the AIO code I have noticed that the way it uses injection points is limited by the fact that we don't have support for runtime parameters in the existing injection point facility. The code is shaped with two set/get functions that set a static parameter that the injection callback would reuse internally, using pgaio_inj_io_get(), and pgaio_io_call_inj() and a static pgaio_inj_cur_handle. Relying on a static variable for that is not a good idea, IMO, even if the stack is reset with a TRY/CATCH block on error in the callback run.
Supporting runtime parameters in injection points is something that I have mentioned as wanted a couple of times, but I was waiting for an actual use-case in core before adding support for it, as mentioned around here: https://www.postgresql.org/message-id/z_yn_galw-pe2...@paquier.xyz test_aio is bringing one. We are still in early April, and I'd like to propose a cleanup of the AIO code on HEAD, even if we are post-feature freeze, to not release this new code in this state, implying an open item. Please note that I'm OK to take responsibility for this patch set at the end, reviews are welcome. Anyway, I have spent some time with my mind on that, and finished with the attached patch set: - 0001 is the addition of runtime parameters in the backend code. I have made the choice of extending the existing INJECTION_POINT() and INJECTION_POINT_CACHED() instead of introducing new macros. That's a matter of taste, perhaps, but increasing the number of these macros leads to a confusing result. This one is more invasive, but that's OK for me. The code is shaped so as we can rely on InjectionPointCallback to define the shape of a callback. It is possible to pass down a full structure if one wants, that the callback is then responsible for translating back. The AIO code only want an AIO handle, which is simple. - 0002 introduces a few updates to the module injection_points, adding support for runtime parameters and some tests. - 0003 cleans up the AOI test code, relying on 0001. The CI is passing. Thoughts and comments are welcome. -- Michael
From 7e8453987d127144477a8e7e9fa7ae61e48091be Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Mon, 14 Apr 2025 16:24:18 +0900
Subject: [PATCH 1/3] Add support for runtime arguments in injection points
The macros INJECTION_POINT() and INJECTION_POINT_CACHED() are extended
with an optional argument that can be passed down to the callback
defined.
---
src/include/utils/injection_point.h | 15 ++++++++-------
src/backend/access/gin/ginbtree.c | 6 +++---
src/backend/access/heap/heapam.c | 2 +-
src/backend/access/index/genam.c | 2 +-
src/backend/access/transam/multixact.c | 4 ++--
src/backend/access/transam/xlog.c | 2 +-
src/backend/commands/indexcmds.c | 4 ++--
src/backend/executor/nodeAgg.c | 10 +++++-----
src/backend/libpq/be-secure-gssapi.c | 2 +-
src/backend/libpq/be-secure.c | 2 +-
src/backend/postmaster/autovacuum.c | 2 +-
src/backend/storage/aio/aio.c | 2 +-
src/backend/tcop/backend_startup.c | 2 +-
src/backend/tcop/postgres.c | 6 +++---
src/backend/utils/cache/catcache.c | 2 +-
src/backend/utils/cache/inval.c | 2 +-
src/backend/utils/cache/typcache.c | 2 +-
src/backend/utils/init/postinit.c | 2 +-
src/backend/utils/misc/injection_point.c | 8 ++++----
.../modules/injection_points/injection_points.c | 4 ++--
src/test/modules/test_slru/test_multixact.c | 2 +-
doc/src/sgml/xfunc.sgml | 11 +++++++----
22 files changed, 49 insertions(+), 45 deletions(-)
diff --git a/src/include/utils/injection_point.h b/src/include/utils/injection_point.h
index 6ba64cd1ebc6..470e1fc582d8 100644
--- a/src/include/utils/injection_point.h
+++ b/src/include/utils/injection_point.h
@@ -16,13 +16,13 @@
*/
#ifdef USE_INJECTION_POINTS
#define INJECTION_POINT_LOAD(name) InjectionPointLoad(name)
-#define INJECTION_POINT(name) InjectionPointRun(name)
-#define INJECTION_POINT_CACHED(name) InjectionPointCached(name)
+#define INJECTION_POINT(name, arg) InjectionPointRun(name, arg)
+#define INJECTION_POINT_CACHED(name, arg) InjectionPointCached(name, arg)
#define IS_INJECTION_POINT_ATTACHED(name) IsInjectionPointAttached(name)
#else
#define INJECTION_POINT_LOAD(name) ((void) name)
-#define INJECTION_POINT(name) ((void) name)
-#define INJECTION_POINT_CACHED(name) ((void) name)
+#define INJECTION_POINT(name, arg) ((void) name)
+#define INJECTION_POINT_CACHED(name, arg) ((void) name)
#define IS_INJECTION_POINT_ATTACHED(name) (false)
#endif
@@ -30,7 +30,8 @@
* Typedef for callback function launched by an injection point.
*/
typedef void (*InjectionPointCallback) (const char *name,
- const void *private_data);
+ const void *private_data,
+ const void *arg);
extern Size InjectionPointShmemSize(void);
extern void InjectionPointShmemInit(void);
@@ -41,8 +42,8 @@ extern void InjectionPointAttach(const char *name,
const void *private_data,
int private_data_size);
extern void InjectionPointLoad(const char *name);
-extern void InjectionPointRun(const char *name);
-extern void InjectionPointCached(const char *name);
+extern void InjectionPointRun(const char *name, const void *arg);
+extern void InjectionPointCached(const char *name, const void *arg);
extern bool IsInjectionPointAttached(const char *name);
extern bool InjectionPointDetach(const char *name);
diff --git a/src/backend/access/gin/ginbtree.c b/src/backend/access/gin/ginbtree.c
index 26a0bdc2063a..644d484ea53c 100644
--- a/src/backend/access/gin/ginbtree.c
+++ b/src/backend/access/gin/ginbtree.c
@@ -685,9 +685,9 @@ ginFinishSplit(GinBtree btree, GinBtreeStack *stack, bool freestack,
#ifdef USE_INJECTION_POINTS
if (GinPageIsLeaf(BufferGetPage(stack->buffer)))
- INJECTION_POINT("gin-leave-leaf-split-incomplete");
+ INJECTION_POINT("gin-leave-leaf-split-incomplete", NULL);
else
- INJECTION_POINT("gin-leave-internal-split-incomplete");
+ INJECTION_POINT("gin-leave-internal-split-incomplete", NULL);
#endif
/* search parent to lock */
@@ -778,7 +778,7 @@ ginFinishSplit(GinBtree btree, GinBtreeStack *stack, bool freestack,
static void
ginFinishOldSplit(GinBtree btree, GinBtreeStack *stack, GinStatsData *buildStats, int access)
{
- INJECTION_POINT("gin-finish-incomplete-split");
+ INJECTION_POINT("gin-finish-incomplete-split", NULL);
elog(DEBUG1, "finishing incomplete split of block %u in gin index \"%s\"",
stack->blkno, RelationGetRelationName(btree->index));
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index ed2e30217992..b01e82ea9712 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -3304,7 +3304,7 @@ heap_update(Relation relation, ItemPointer otid, HeapTuple newtup,
interesting_attrs = bms_add_members(interesting_attrs, id_attrs);
block = ItemPointerGetBlockNumber(otid);
- INJECTION_POINT("heap_update-before-pin");
+ INJECTION_POINT("heap_update-before-pin", NULL);
buffer = ReadBuffer(relation, block);
page = BufferGetPage(buffer);
diff --git a/src/backend/access/index/genam.c b/src/backend/access/index/genam.c
index 8f532e14590e..0cb27af13109 100644
--- a/src/backend/access/index/genam.c
+++ b/src/backend/access/index/genam.c
@@ -851,7 +851,7 @@ systable_inplace_update_begin(Relation relation,
if (retries++ > 10000)
elog(ERROR, "giving up after too many tries to overwrite row");
- INJECTION_POINT("inplace-before-pin");
+ INJECTION_POINT("inplace-before-pin", NULL);
scan = systable_beginscan(relation, indexId, indexOK, snapshot,
nkeys, unconstify(ScanKeyData *, key));
oldtup = systable_getnext(scan);
diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c
index 9d25a7df0d32..7a827df354f8 100644
--- a/src/backend/access/transam/multixact.c
+++ b/src/backend/access/transam/multixact.c
@@ -872,7 +872,7 @@ MultiXactIdCreateFromMembers(int nmembers, MultiXactMember *members)
*/
multi = GetNewMultiXactId(nmembers, &offset);
- INJECTION_POINT_CACHED("multixact-create-from-members");
+ INJECTION_POINT_CACHED("multixact-create-from-members", NULL);
/* Make an XLOG entry describing the new MXID. */
xlrec.mid = multi;
@@ -1486,7 +1486,7 @@ retry:
LWLockRelease(lock);
CHECK_FOR_INTERRUPTS();
- INJECTION_POINT("multixact-get-members-cv-sleep");
+ INJECTION_POINT("multixact-get-members-cv-sleep", NULL);
ConditionVariableSleep(&MultiXactState->nextoff_cv,
WAIT_EVENT_MULTIXACT_CREATION);
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index ec40c0b7c42b..207f68259301 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -7882,7 +7882,7 @@ CreateRestartPoint(int flags)
* This location needs to be after CheckPointGuts() to ensure that some
* work has already happened during this checkpoint.
*/
- INJECTION_POINT("create-restart-point");
+ INJECTION_POINT("create-restart-point", NULL);
/*
* Remember the prior checkpoint's redo ptr for
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 33c2106c17ce..d962fe392cd2 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -3892,9 +3892,9 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein
#ifdef USE_INJECTION_POINTS
if (idx->safe)
- INJECTION_POINT("reindex-conc-index-safe");
+ INJECTION_POINT("reindex-conc-index-safe", NULL);
else
- INJECTION_POINT("reindex-conc-index-not-safe");
+ INJECTION_POINT("reindex-conc-index-not-safe", NULL);
#endif
idx->tableId = RelationGetRelid(heapRel);
diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c
index f83fc16c5c8e..377e016d7322 100644
--- a/src/backend/executor/nodeAgg.c
+++ b/src/backend/executor/nodeAgg.c
@@ -1492,7 +1492,7 @@ build_hash_tables(AggState *aggstate)
if (IS_INJECTION_POINT_ATTACHED("hash-aggregate-oversize-table"))
{
nbuckets = memory / TupleHashEntrySize();
- INJECTION_POINT_CACHED("hash-aggregate-oversize-table");
+ INJECTION_POINT_CACHED("hash-aggregate-oversize-table", NULL);
}
#endif
@@ -1882,7 +1882,7 @@ hash_agg_check_limits(AggState *aggstate)
if (IS_INJECTION_POINT_ATTACHED("hash-aggregate-spill-1000"))
{
do_spill = true;
- INJECTION_POINT_CACHED("hash-aggregate-spill-1000");
+ INJECTION_POINT_CACHED("hash-aggregate-spill-1000", NULL);
}
}
#endif
@@ -1910,7 +1910,7 @@ hash_agg_check_limits(AggState *aggstate)
static void
hash_agg_enter_spill_mode(AggState *aggstate)
{
- INJECTION_POINT("hash-aggregate-enter-spill-mode");
+ INJECTION_POINT("hash-aggregate-enter-spill-mode", NULL);
aggstate->hash_spill_mode = true;
hashagg_recompile_expressions(aggstate, aggstate->table_filled, true);
@@ -2739,7 +2739,7 @@ agg_refill_hash_table(AggState *aggstate)
*/
hashagg_recompile_expressions(aggstate, true, true);
- INJECTION_POINT("hash-aggregate-process-batch");
+ INJECTION_POINT("hash-aggregate-process-batch", NULL);
for (;;)
{
TupleTableSlot *spillslot = aggstate->hash_spill_rslot;
@@ -2995,7 +2995,7 @@ hashagg_spill_init(HashAggSpill *spill, LogicalTapeSet *tapeset, int used_bits,
{
npartitions = 1;
partition_bits = 0;
- INJECTION_POINT_CACHED("hash-aggregate-single-partition");
+ INJECTION_POINT_CACHED("hash-aggregate-single-partition", NULL);
}
#endif
diff --git a/src/backend/libpq/be-secure-gssapi.c b/src/backend/libpq/be-secure-gssapi.c
index a3d610b1373d..717ba9824f91 100644
--- a/src/backend/libpq/be-secure-gssapi.c
+++ b/src/backend/libpq/be-secure-gssapi.c
@@ -500,7 +500,7 @@ secure_open_gssapi(Port *port)
minor;
gss_cred_id_t delegated_creds;
- INJECTION_POINT("backend-gssapi-startup");
+ INJECTION_POINT("backend-gssapi-startup", NULL);
/*
* Allocate subsidiary Port data for GSSAPI operations.
diff --git a/src/backend/libpq/be-secure.c b/src/backend/libpq/be-secure.c
index 91576f942857..d723e74e8137 100644
--- a/src/backend/libpq/be-secure.c
+++ b/src/backend/libpq/be-secure.c
@@ -131,7 +131,7 @@ secure_open_server(Port *port)
}
Assert(pq_buffer_remaining_data() == 0);
- INJECTION_POINT("backend-ssl-startup");
+ INJECTION_POINT("backend-ssl-startup", NULL);
r = be_tls_open_server(port);
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 16756152b71a..4d4a1a3197ec 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -1922,7 +1922,7 @@ do_autovacuum(void)
* This injection point is put in a transaction block to work with a wait
* that uses a condition variable.
*/
- INJECTION_POINT("autovacuum-worker-start");
+ INJECTION_POINT("autovacuum-worker-start", NULL);
/*
* Compute the multixact age for which freezing is urgent. This is
diff --git a/src/backend/storage/aio/aio.c b/src/backend/storage/aio/aio.c
index 86f7250b7a5f..366cbc23d6b7 100644
--- a/src/backend/storage/aio/aio.c
+++ b/src/backend/storage/aio/aio.c
@@ -1245,7 +1245,7 @@ pgaio_io_call_inj(PgAioHandle *ioh, const char *injection_point)
PG_TRY();
{
- InjectionPointCached(injection_point);
+ InjectionPointCached(injection_point, NULL);
}
PG_FINALLY();
{
diff --git a/src/backend/tcop/backend_startup.c b/src/backend/tcop/backend_startup.c
index dde8d5b35175..a7d1fec981f8 100644
--- a/src/backend/tcop/backend_startup.c
+++ b/src/backend/tcop/backend_startup.c
@@ -234,7 +234,7 @@ BackendInitialize(ClientSocket *client_sock, CAC_state cac)
/* For testing client error handling */
#ifdef USE_INJECTION_POINTS
- INJECTION_POINT("backend-initialize");
+ INJECTION_POINT("backend-initialize", NULL);
if (IS_INJECTION_POINT_ATTACHED("backend-initialize-v2-error"))
{
/*
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index dc4c600922df..1ae51b1b3915 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -3482,7 +3482,7 @@ ProcessInterrupts(void)
IdleInTransactionSessionTimeoutPending = false;
if (IdleInTransactionSessionTimeout > 0)
{
- INJECTION_POINT("idle-in-transaction-session-timeout");
+ INJECTION_POINT("idle-in-transaction-session-timeout", NULL);
ereport(FATAL,
(errcode(ERRCODE_IDLE_IN_TRANSACTION_SESSION_TIMEOUT),
errmsg("terminating connection due to idle-in-transaction timeout")));
@@ -3495,7 +3495,7 @@ ProcessInterrupts(void)
TransactionTimeoutPending = false;
if (TransactionTimeout > 0)
{
- INJECTION_POINT("transaction-timeout");
+ INJECTION_POINT("transaction-timeout", NULL);
ereport(FATAL,
(errcode(ERRCODE_TRANSACTION_TIMEOUT),
errmsg("terminating connection due to transaction timeout")));
@@ -3508,7 +3508,7 @@ ProcessInterrupts(void)
IdleSessionTimeoutPending = false;
if (IdleSessionTimeout > 0)
{
- INJECTION_POINT("idle-session-timeout");
+ INJECTION_POINT("idle-session-timeout", NULL);
ereport(FATAL,
(errcode(ERRCODE_IDLE_SESSION_TIMEOUT),
errmsg("terminating connection due to idle-session timeout")));
diff --git a/src/backend/utils/cache/catcache.c b/src/backend/utils/cache/catcache.c
index 9ad7681f1555..13b6f104146d 100644
--- a/src/backend/utils/cache/catcache.c
+++ b/src/backend/utils/cache/catcache.c
@@ -1904,7 +1904,7 @@ SearchCatCacheList(CatCache *cache,
/* Injection point to help testing the recursive invalidation case */
if (first_iter)
{
- INJECTION_POINT("catcache-list-miss-systable-scan-started");
+ INJECTION_POINT("catcache-list-miss-systable-scan-started", NULL);
first_iter = false;
}
diff --git a/src/backend/utils/cache/inval.c b/src/backend/utils/cache/inval.c
index 4893dbdd7c8b..5f5e5c9f9d08 100644
--- a/src/backend/utils/cache/inval.c
+++ b/src/backend/utils/cache/inval.c
@@ -1200,7 +1200,7 @@ AtEOXact_Inval(bool isCommit)
/* Must be at top of stack */
Assert(transInvalInfo->my_level == 1 && transInvalInfo->parent == NULL);
- INJECTION_POINT("AtEOXact_Inval-with-transInvalInfo");
+ INJECTION_POINT("AtEOXact_Inval-with-transInvalInfo", NULL);
if (isCommit)
{
diff --git a/src/backend/utils/cache/typcache.c b/src/backend/utils/cache/typcache.c
index ae65a1cce06c..07e4e6284958 100644
--- a/src/backend/utils/cache/typcache.c
+++ b/src/backend/utils/cache/typcache.c
@@ -952,7 +952,7 @@ lookup_type_cache(Oid type_id, int flags)
load_domaintype_info(typentry);
}
- INJECTION_POINT("typecache-before-rel-type-cache-insert");
+ INJECTION_POINT("typecache-before-rel-type-cache-insert", NULL);
Assert(in_progress_offset + 1 == in_progress_list_len);
in_progress_list_len--;
diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
index 01309ef3f867..b7856670a0db 100644
--- a/src/backend/utils/init/postinit.c
+++ b/src/backend/utils/init/postinit.c
@@ -751,7 +751,7 @@ InitPostgres(const char *in_dbname, Oid dboid,
if (!bootstrap)
{
pgstat_bestart_initial();
- INJECTION_POINT("init-pre-auth");
+ INJECTION_POINT("init-pre-auth", NULL);
}
/*
diff --git a/src/backend/utils/misc/injection_point.c b/src/backend/utils/misc/injection_point.c
index 97ab851f0a63..389a71910ed7 100644
--- a/src/backend/utils/misc/injection_point.c
+++ b/src/backend/utils/misc/injection_point.c
@@ -541,14 +541,14 @@ InjectionPointLoad(const char *name)
* Execute an injection point, if defined.
*/
void
-InjectionPointRun(const char *name)
+InjectionPointRun(const char *name, const void *arg)
{
#ifdef USE_INJECTION_POINTS
InjectionPointCacheEntry *cache_entry;
cache_entry = InjectionPointCacheRefresh(name);
if (cache_entry)
- cache_entry->callback(name, cache_entry->private_data);
+ cache_entry->callback(name, cache_entry->private_data, arg);
#else
elog(ERROR, "Injection points are not supported by this build");
#endif
@@ -558,14 +558,14 @@ InjectionPointRun(const char *name)
* Execute an injection point directly from the cache, if defined.
*/
void
-InjectionPointCached(const char *name)
+InjectionPointCached(const char *name, const void *arg)
{
#ifdef USE_INJECTION_POINTS
InjectionPointCacheEntry *cache_entry;
cache_entry = injection_point_cache_get(name);
if (cache_entry)
- cache_entry->callback(name, cache_entry->private_data);
+ cache_entry->callback(name, cache_entry->private_data, arg);
#else
elog(ERROR, "Injection points are not supported by this build");
#endif
diff --git a/src/test/modules/injection_points/injection_points.c b/src/test/modules/injection_points/injection_points.c
index ad528d77752b..027ef56964d1 100644
--- a/src/test/modules/injection_points/injection_points.c
+++ b/src/test/modules/injection_points/injection_points.c
@@ -405,7 +405,7 @@ injection_points_run(PG_FUNCTION_ARGS)
char *name = text_to_cstring(PG_GETARG_TEXT_PP(0));
pgstat_report_inj_fixed(0, 0, 1, 0, 0);
- INJECTION_POINT(name);
+ INJECTION_POINT(name, NULL);
PG_RETURN_VOID();
}
@@ -420,7 +420,7 @@ injection_points_cached(PG_FUNCTION_ARGS)
char *name = text_to_cstring(PG_GETARG_TEXT_PP(0));
pgstat_report_inj_fixed(0, 0, 0, 1, 0);
- INJECTION_POINT_CACHED(name);
+ INJECTION_POINT_CACHED(name, NULL);
PG_RETURN_VOID();
}
diff --git a/src/test/modules/test_slru/test_multixact.c b/src/test/modules/test_slru/test_multixact.c
index a9b9628c9587..6c9b0420717c 100644
--- a/src/test/modules/test_slru/test_multixact.c
+++ b/src/test/modules/test_slru/test_multixact.c
@@ -46,7 +46,7 @@ test_read_multixact(PG_FUNCTION_ARGS)
MultiXactId id = PG_GETARG_TRANSACTIONID(0);
MultiXactMember *members;
- INJECTION_POINT("test-multixact-read");
+ INJECTION_POINT("test-multixact-read", NULL);
/* discard caches */
AtEOXact_MultiXact();
diff --git a/doc/src/sgml/xfunc.sgml b/doc/src/sgml/xfunc.sgml
index 8074f66417da..451af81b8523 100644
--- a/doc/src/sgml/xfunc.sgml
+++ b/doc/src/sgml/xfunc.sgml
@@ -3829,14 +3829,15 @@ uint32 WaitEventExtensionNew(const char *wait_event_name)
An injection point with a given <literal>name</literal> is declared using
macro:
<programlisting>
-INJECTION_POINT(name);
+INJECTION_POINT(name, arg);
</programlisting>
There are a few injection points already declared at strategic points
within the server code. After adding a new injection point the code needs
to be compiled in order for that injection point to be available in the
binary. Add-ins written in C-language can declare injection points in
- their own code using the same macro.
+ their own code using the same macro. <literal>arg</literal> is an optional
+ argument value given to the callback at run-time.
</para>
<para>
@@ -3846,7 +3847,7 @@ INJECTION_POINT(name);
a two-step approach with the following macros:
<programlisting>
INJECTION_POINT_LOAD(name);
-INJECTION_POINT_CACHED(name);
+INJECTION_POINT_CACHED(name, arg);
</programlisting>
Before entering the critical section,
@@ -3879,7 +3880,9 @@ extern void InjectionPointAttach(const char *name,
<literal>InjectionPointCallback</literal>:
<programlisting>
static void
-custom_injection_callback(const char *name, const void *private_data)
+custom_injection_callback(const char *name,
+ const void *private_data,
+ const void *arg)
{
uint32 wait_event_info = WaitEventInjectionPointNew(name);
--
2.49.0
From 7dbeb7f25bad05d1a4584913c84181e3ad410dc3 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Mon, 14 Apr 2025 15:46:32 +0900
Subject: [PATCH 2/3] injection_points: Add support for runtime arguments
This provides test coverage for run-time arguments for the cached and
run paths.
---
.../expected/injection_points.out | 45 ++++++++++++++++
.../injection_points--1.0.sql | 10 ++--
.../injection_points/injection_points.c | 53 ++++++++++++++-----
.../injection_points/sql/injection_points.sql | 10 ++++
4 files changed, 102 insertions(+), 16 deletions(-)
diff --git a/src/test/modules/injection_points/expected/injection_points.out b/src/test/modules/injection_points/expected/injection_points.out
index f25bbe4966ee..43bcdd01582f 100644
--- a/src/test/modules/injection_points/expected/injection_points.out
+++ b/src/test/modules/injection_points/expected/injection_points.out
@@ -6,6 +6,19 @@ CREATE FUNCTION wait_pid(int)
RETURNS void
AS :'regresslib'
LANGUAGE C STRICT;
+-- Non-strict checks
+SELECT injection_points_run(NULL);
+ injection_points_run
+----------------------
+
+(1 row)
+
+SELECT injection_points_cached(NULL);
+ injection_points_cached
+-------------------------
+
+(1 row)
+
SELECT injection_points_attach('TestInjectionBooh', 'booh');
ERROR: incorrect action "booh" for injection point creation
SELECT injection_points_attach('TestInjectionError', 'error');
@@ -39,6 +52,20 @@ NOTICE: notice triggered for injection point TestInjectionLog2
(1 row)
+SELECT injection_points_run('TestInjectionLog2', NULL); -- notice
+NOTICE: notice triggered for injection point TestInjectionLog2
+ injection_points_run
+----------------------
+
+(1 row)
+
+SELECT injection_points_run('TestInjectionLog2', 'foobar'); -- notice + arg
+NOTICE: notice triggered for injection point TestInjectionLog2 (foobar)
+ injection_points_run
+----------------------
+
+(1 row)
+
SELECT injection_points_run('TestInjectionLog'); -- notice
NOTICE: notice triggered for injection point TestInjectionLog
injection_points_run
@@ -48,6 +75,10 @@ NOTICE: notice triggered for injection point TestInjectionLog
SELECT injection_points_run('TestInjectionError'); -- error
ERROR: error triggered for injection point TestInjectionError
+SELECT injection_points_run('TestInjectionError', NULL); -- error
+ERROR: error triggered for injection point TestInjectionError
+SELECT injection_points_run('TestInjectionError', 'foobar2'); -- error + arg
+ERROR: error triggered for injection point TestInjectionError (foobar2)
-- Re-load cache and run again.
\c
SELECT injection_points_run('TestInjectionLog2'); -- notice
@@ -160,6 +191,20 @@ NOTICE: notice triggered for injection point TestInjectionLogLoad
(1 row)
+SELECT injection_points_cached('TestInjectionLogLoad', NULL); -- runs from cache
+NOTICE: notice triggered for injection point TestInjectionLogLoad
+ injection_points_cached
+-------------------------
+
+(1 row)
+
+SELECT injection_points_cached('TestInjectionLogLoad', 'foobar'); -- runs from cache
+NOTICE: notice triggered for injection point TestInjectionLogLoad (foobar)
+ injection_points_cached
+-------------------------
+
+(1 row)
+
SELECT injection_points_run('TestInjectionLogLoad'); -- runs from cache
NOTICE: notice triggered for injection point TestInjectionLogLoad
injection_points_run
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 5d83f08811b0..cc76b1bf99ae 100644
--- a/src/test/modules/injection_points/injection_points--1.0.sql
+++ b/src/test/modules/injection_points/injection_points--1.0.sql
@@ -29,20 +29,22 @@ LANGUAGE C STRICT PARALLEL UNSAFE;
--
-- Executes the action attached to the injection point.
--
-CREATE FUNCTION injection_points_run(IN point_name TEXT)
+CREATE FUNCTION injection_points_run(IN point_name TEXT,
+ IN arg TEXT DEFAULT NULL)
RETURNS void
AS 'MODULE_PATHNAME', 'injection_points_run'
-LANGUAGE C STRICT PARALLEL UNSAFE;
+LANGUAGE C PARALLEL UNSAFE;
--
-- injection_points_cached()
--
-- Executes the action attached to the injection point, from local cache.
--
-CREATE FUNCTION injection_points_cached(IN point_name TEXT)
+CREATE FUNCTION injection_points_cached(IN point_name TEXT,
+ IN arg TEXT DEFAULT NULL)
RETURNS void
AS 'MODULE_PATHNAME', 'injection_points_cached'
-LANGUAGE C STRICT PARALLEL UNSAFE;
+LANGUAGE C PARALLEL UNSAFE;
--
-- injection_points_wakeup()
diff --git a/src/test/modules/injection_points/injection_points.c b/src/test/modules/injection_points/injection_points.c
index 027ef56964d1..4187f3096dd4 100644
--- a/src/test/modules/injection_points/injection_points.c
+++ b/src/test/modules/injection_points/injection_points.c
@@ -94,11 +94,14 @@ typedef struct InjectionPointSharedState
static InjectionPointSharedState *inj_state = NULL;
extern PGDLLEXPORT void injection_error(const char *name,
- const void *private_data);
+ const void *private_data,
+ const void *arg);
extern PGDLLEXPORT void injection_notice(const char *name,
- const void *private_data);
+ const void *private_data,
+ const void *arg);
extern PGDLLEXPORT void injection_wait(const char *name,
- const void *private_data);
+ const void *private_data,
+ const void *arg);
/* track if injection points attached in this process are linked to it */
static bool injection_point_local = false;
@@ -239,34 +242,44 @@ injection_points_cleanup(int code, Datum arg)
/* Set of callbacks available to be attached to an injection point. */
void
-injection_error(const char *name, const void *private_data)
+injection_error(const char *name, const void *private_data, const void *arg)
{
InjectionPointCondition *condition = (InjectionPointCondition *) private_data;
+ char *argstr = (char *) arg;
if (!injection_point_allowed(condition))
return;
pgstat_report_inj(name);
- elog(ERROR, "error triggered for injection point %s", name);
+ if (argstr)
+ elog(ERROR, "error triggered for injection point %s (%s)",
+ name, argstr);
+ else
+ elog(ERROR, "error triggered for injection point %s", name);
}
void
-injection_notice(const char *name, const void *private_data)
+injection_notice(const char *name, const void *private_data, const void *arg)
{
InjectionPointCondition *condition = (InjectionPointCondition *) private_data;
+ char *argstr = (char *) arg;
if (!injection_point_allowed(condition))
return;
pgstat_report_inj(name);
- elog(NOTICE, "notice triggered for injection point %s", name);
+ if (argstr)
+ elog(NOTICE, "notice triggered for injection point %s (%s)",
+ name, argstr);
+ else
+ elog(NOTICE, "notice triggered for injection point %s", name);
}
/* Wait on a condition variable, awaken by injection_points_wakeup() */
void
-injection_wait(const char *name, const void *private_data)
+injection_wait(const char *name, const void *private_data, const void *arg)
{
uint32 old_wait_counts = 0;
int index = -1;
@@ -402,10 +415,18 @@ PG_FUNCTION_INFO_V1(injection_points_run);
Datum
injection_points_run(PG_FUNCTION_ARGS)
{
- char *name = text_to_cstring(PG_GETARG_TEXT_PP(0));
+ char *name;
+ char *arg = NULL;
+
+ if (PG_ARGISNULL(0))
+ PG_RETURN_VOID();
+ name = text_to_cstring(PG_GETARG_TEXT_PP(0));
+
+ if (!PG_ARGISNULL(1))
+ arg = text_to_cstring(PG_GETARG_TEXT_PP(1));
pgstat_report_inj_fixed(0, 0, 1, 0, 0);
- INJECTION_POINT(name, NULL);
+ INJECTION_POINT(name, arg);
PG_RETURN_VOID();
}
@@ -417,10 +438,18 @@ PG_FUNCTION_INFO_V1(injection_points_cached);
Datum
injection_points_cached(PG_FUNCTION_ARGS)
{
- char *name = text_to_cstring(PG_GETARG_TEXT_PP(0));
+ char *name;
+ char *arg = NULL;
+
+ if (PG_ARGISNULL(0))
+ PG_RETURN_VOID();
+ name = text_to_cstring(PG_GETARG_TEXT_PP(0));
+
+ if (!PG_ARGISNULL(1))
+ arg = text_to_cstring(PG_GETARG_TEXT_PP(1));
pgstat_report_inj_fixed(0, 0, 0, 1, 0);
- INJECTION_POINT_CACHED(name, NULL);
+ INJECTION_POINT_CACHED(name, arg);
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 e3a481d60449..d9748331c771 100644
--- a/src/test/modules/injection_points/sql/injection_points.sql
+++ b/src/test/modules/injection_points/sql/injection_points.sql
@@ -9,6 +9,10 @@ CREATE FUNCTION wait_pid(int)
AS :'regresslib'
LANGUAGE C STRICT;
+-- Non-strict checks
+SELECT injection_points_run(NULL);
+SELECT injection_points_cached(NULL);
+
SELECT injection_points_attach('TestInjectionBooh', 'booh');
SELECT injection_points_attach('TestInjectionError', 'error');
SELECT injection_points_attach('TestInjectionLog', 'notice');
@@ -16,8 +20,12 @@ SELECT injection_points_attach('TestInjectionLog2', 'notice');
SELECT injection_points_run('TestInjectionBooh'); -- nothing
SELECT injection_points_run('TestInjectionLog2'); -- notice
+SELECT injection_points_run('TestInjectionLog2', NULL); -- notice
+SELECT injection_points_run('TestInjectionLog2', 'foobar'); -- notice + arg
SELECT injection_points_run('TestInjectionLog'); -- notice
SELECT injection_points_run('TestInjectionError'); -- error
+SELECT injection_points_run('TestInjectionError', NULL); -- error
+SELECT injection_points_run('TestInjectionError', 'foobar2'); -- error + arg
-- Re-load cache and run again.
\c
@@ -47,6 +55,8 @@ SELECT injection_points_load('TestInjectionLogLoad'); -- nothing
SELECT injection_points_attach('TestInjectionLogLoad', 'notice');
SELECT injection_points_load('TestInjectionLogLoad'); -- nothing happens
SELECT injection_points_cached('TestInjectionLogLoad'); -- runs from cache
+SELECT injection_points_cached('TestInjectionLogLoad', NULL); -- runs from cache
+SELECT injection_points_cached('TestInjectionLogLoad', 'foobar'); -- runs from cache
SELECT injection_points_run('TestInjectionLogLoad'); -- runs from cache
SELECT injection_points_detach('TestInjectionLogLoad');
--
2.49.0
From f47e88ba13bce1857f61d5b5d4517640a754f8ad Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Mon, 14 Apr 2025 16:24:54 +0900
Subject: [PATCH 3/3] aio: Use runtime arguments with injections points
This cleans up some code related to AIO for injection points, switching
the test code to use the in-core support rather than tweaks to pass
arguments to the callbacks run.
---
src/include/storage/aio_internal.h | 20 ---------
src/backend/storage/aio/aio.c | 58 +------------------------
src/backend/storage/aio/method_worker.c | 3 +-
src/test/modules/test_aio/test_aio.c | 16 ++++---
4 files changed, 13 insertions(+), 84 deletions(-)
diff --git a/src/include/storage/aio_internal.h b/src/include/storage/aio_internal.h
index 33f27b9fe508..2d37a243abe5 100644
--- a/src/include/storage/aio_internal.h
+++ b/src/include/storage/aio_internal.h
@@ -394,26 +394,6 @@ extern const char *pgaio_io_get_target_name(PgAioHandle *ioh);
pgaio_io_get_state_name(ioh), \
__VA_ARGS__)
-
-#ifdef USE_INJECTION_POINTS
-
-extern void pgaio_io_call_inj(PgAioHandle *ioh, const char *injection_point);
-
-/* just for use in tests, from within injection points */
-extern PgAioHandle *pgaio_inj_io_get(void);
-
-#else
-
-#define pgaio_io_call_inj(ioh, injection_point) (void) 0
-
-/*
- * no fallback for pgaio_inj_io_get, all code using injection points better be
- * guarded by USE_INJECTION_POINTS.
- */
-
-#endif
-
-
/* Declarations for the tables of function pointers exposed by each IO method. */
extern PGDLLIMPORT const IoMethodOps pgaio_sync_ops;
extern PGDLLIMPORT const IoMethodOps pgaio_worker_ops;
diff --git a/src/backend/storage/aio/aio.c b/src/backend/storage/aio/aio.c
index 366cbc23d6b7..cfb2b3301106 100644
--- a/src/backend/storage/aio/aio.c
+++ b/src/backend/storage/aio/aio.c
@@ -46,13 +46,10 @@
#include "storage/aio_subsys.h"
#include "utils/guc.h"
#include "utils/guc_hooks.h"
+#include "utils/injection_point.h"
#include "utils/resowner.h"
#include "utils/wait_event_types.h"
-#ifdef USE_INJECTION_POINTS
-#include "utils/injection_point.h"
-#endif
-
static inline void pgaio_io_update_state(PgAioHandle *ioh, PgAioHandleState new_state);
static void pgaio_io_reclaim(PgAioHandle *ioh);
@@ -96,17 +93,6 @@ static const IoMethodOps *const pgaio_method_ops_table[] = {
const IoMethodOps *pgaio_method_ops;
-/*
- * Currently there's no infrastructure to pass arguments to injection points,
- * so we instead set this up for the duration of the injection point
- * invocation. See pgaio_io_call_inj().
- */
-#ifdef USE_INJECTION_POINTS
-static PgAioHandle *pgaio_inj_cur_handle;
-#endif
-
-
-
/* --------------------------------------------------------------------------------
* Public Functions related to PgAioHandle
* --------------------------------------------------------------------------------
@@ -507,7 +493,7 @@ pgaio_io_process_completion(PgAioHandle *ioh, int result)
pgaio_io_update_state(ioh, PGAIO_HS_COMPLETED_IO);
- pgaio_io_call_inj(ioh, "AIO_PROCESS_COMPLETION_BEFORE_SHARED");
+ INJECTION_POINT("AIO_PROCESS_COMPLETION_BEFORE_SHARED", ioh);
pgaio_io_call_complete_shared(ioh);
@@ -1225,43 +1211,3 @@ check_io_max_concurrency(int *newval, void **extra, GucSource source)
return true;
}
-
-
-
-/* --------------------------------------------------------------------------------
- * Injection point support
- * --------------------------------------------------------------------------------
- */
-
-#ifdef USE_INJECTION_POINTS
-
-/*
- * Call injection point with support for pgaio_inj_io_get().
- */
-void
-pgaio_io_call_inj(PgAioHandle *ioh, const char *injection_point)
-{
- pgaio_inj_cur_handle = ioh;
-
- PG_TRY();
- {
- InjectionPointCached(injection_point, NULL);
- }
- PG_FINALLY();
- {
- pgaio_inj_cur_handle = NULL;
- }
- PG_END_TRY();
-}
-
-/*
- * Return IO associated with injection point invocation. This is only needed
- * as injection points currently don't support arguments.
- */
-PgAioHandle *
-pgaio_inj_io_get(void)
-{
- return pgaio_inj_cur_handle;
-}
-
-#endif
diff --git a/src/backend/storage/aio/method_worker.c b/src/backend/storage/aio/method_worker.c
index 8ad17ec1ef73..6779fb9b7f77 100644
--- a/src/backend/storage/aio/method_worker.c
+++ b/src/backend/storage/aio/method_worker.c
@@ -42,6 +42,7 @@
#include "storage/latch.h"
#include "storage/proc.h"
#include "tcop/tcopprot.h"
+#include "utils/injection_point.h"
#include "utils/memdebug.h"
#include "utils/ps_status.h"
#include "utils/wait_event.h"
@@ -525,7 +526,7 @@ IoWorkerMain(const void *startup_data, size_t startup_data_len)
* To be able to exercise the reopen-fails path, allow injection
* points to trigger a failure at this point.
*/
- pgaio_io_call_inj(ioh, "AIO_WORKER_AFTER_REOPEN");
+ INJECTION_POINT("AIO_WORKER_AFTER_REOPEN", ioh);
error_errno = 0;
error_ioh = NULL;
diff --git a/src/test/modules/test_aio/test_aio.c b/src/test/modules/test_aio/test_aio.c
index 1d776010ef41..07711e46ecbf 100644
--- a/src/test/modules/test_aio/test_aio.c
+++ b/src/test/modules/test_aio/test_aio.c
@@ -674,13 +674,17 @@ batch_end(PG_FUNCTION_ARGS)
}
#ifdef USE_INJECTION_POINTS
-extern PGDLLEXPORT void inj_io_short_read(const char *name, const void *private_data);
-extern PGDLLEXPORT void inj_io_reopen(const char *name, const void *private_data);
+extern PGDLLEXPORT void inj_io_short_read(const char *name,
+ const void *private_data,
+ const void *arg);
+extern PGDLLEXPORT void inj_io_reopen(const char *name,
+ const void *private_data,
+ const void *arg);
void
-inj_io_short_read(const char *name, const void *private_data)
+inj_io_short_read(const char *name, const void *private_data, const void *arg)
{
- PgAioHandle *ioh;
+ PgAioHandle *ioh = (PgAioHandle *) arg;
ereport(LOG,
errmsg("short read injection point called, is enabled: %d",
@@ -689,8 +693,6 @@ inj_io_short_read(const char *name, const void *private_data)
if (inj_io_error_state->enabled_short_read)
{
- ioh = pgaio_inj_io_get();
-
/*
* Only shorten reads that are actually longer than the target size,
* otherwise we can trigger over-reads.
@@ -742,7 +744,7 @@ inj_io_short_read(const char *name, const void *private_data)
}
void
-inj_io_reopen(const char *name, const void *private_data)
+inj_io_reopen(const char *name, const void *private_data, const void *arg)
{
ereport(LOG,
errmsg("reopen injection point called, is enabled: %d",
--
2.49.0
signature.asc
Description: PGP signature
