On Fri, May 09, 2025 at 10:48:45AM +0900, Michael Paquier wrote: > Thanks for the input. Will proceed if there are no objections, then.
By the way, Greg, v2-0003 in the patch set you have posted was incorrect; it missed most of the wanted changes to get rid of the workarounds in the AIO code. Compared to the original version, there were just two conflicts with the injection point names, nothing huge. Patch v2 seems to be hold on moderation perhaps? It has not been published to the lists. I will not be able to monitor the buildfarm today but I have the attached v3 staged for commit, for later when I'll be able to do so. -- Michael
From b8d9c97c5ae9a432cc93741efffaf889adb5723d Mon Sep 17 00:00:00 2001 From: Michael Paquier <mich...@paquier.xyz> Date: Fri, 9 May 2025 15:13:57 +0900 Subject: [PATCH v3 1/3] Add support for runtime parameters 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 attached, giving to callbacks the possibility to manipulate a state given by the caller. 93bc3d75d8e1 (test_aio) and da7226993fd4 (core AIO infrastructure) have been relying on set of workarounds where pgaio_inj_cur_handle is used as runtime argument given to the injection point callbacks used by the AIO tests, in combination with a TRY/CATCH block to reset the argument value. The infrastructure introduced in this commit will be reused for the AIO tests, simplifying it. Reviewed-by: Greg Burd <g...@burd.me> Discussion: https://postgr.es/m/z_y9ttnxubvya...@paquier.xyz --- 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 | 16 ++++++++++------ 22 files changed, 52 insertions(+), 47 deletions(-) diff --git a/src/include/utils/injection_point.h b/src/include/utils/injection_point.h index 6ba64cd1ebc6..a37958e1835f 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, + 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, void *arg); +extern void InjectionPointCached(const char *name, 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 c1a4de14a59e..9ec8cda1c680 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 ef91e70f41dc..3c06ac45532f 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 2d4c346473b7..1914859b2eed 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 57b9cf3dcab3..04268e77ec2e 100644 --- a/src/backend/storage/aio/aio.c +++ b/src/backend/storage/aio/aio.c @@ -1275,7 +1275,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 6e3cad454c0a..657648996c23 100644 --- a/src/backend/utils/cache/catcache.c +++ b/src/backend/utils/cache/catcache.c @@ -1926,7 +1926,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 fa7b4d7e303f..02505c88b8e4 100644 --- a/src/backend/utils/cache/inval.c +++ b/src/backend/utils/cache/inval.c @@ -1207,7 +1207,7 @@ AtEOXact_Inval(bool isCommit) /* Must be at top of stack */ Assert(transInvalInfo->my_level == 1 && transInvalInfo->parent == NULL); - INJECTION_POINT("transaction-end-process-inval"); + INJECTION_POINT("transaction-end-process-inval", NULL); if (isCommit) { diff --git a/src/backend/utils/cache/typcache.c b/src/backend/utils/cache/typcache.c index e359da09ec9f..f9aec38a11fb 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 28f09a27001b..89d72cdd5ff5 100644 --- a/src/backend/utils/init/postinit.c +++ b/src/backend/utils/init/postinit.c @@ -747,7 +747,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..f58ebc8ee522 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, 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, 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 3a73b02ccafa..2d81afce8cb9 100644 --- a/doc/src/sgml/xfunc.sgml +++ b/doc/src/sgml/xfunc.sgml @@ -3829,15 +3829,17 @@ 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. The injection point names should - use lower-case characters, with terms separated by dashes. + their own code using the same macro. The injection point names should use + lower-case characters, with terms separated by + dashes. <literal>arg</literal> is an optional argument value given to the + callback at run-time. </para> <para> @@ -3847,7 +3849,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, @@ -3880,7 +3882,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, + void *arg) { uint32 wait_event_info = WaitEventInjectionPointNew(name); @@ -3909,7 +3913,7 @@ if (IS_INJECTION_POINT_ATTACHED("before-foobar")) local_var = 123; /* also execute the callback */ - INJECTION_POINT_CACHED("before-foobar"); + INJECTION_POINT_CACHED("before-foobar", NULL); } #endif </programlisting> -- 2.49.0
From debc4e2a62b9aa4940ba848e96a191d86c9b4743 Mon Sep 17 00:00:00 2001 From: Michael Paquier <mich...@paquier.xyz> Date: Fri, 9 May 2025 15:27:15 +0900 Subject: [PATCH v3 2/3] injection_points: Add support for runtime arguments This commit provides some test coverage for the runtime arguments of injection points, for both INJECTION_POINT_CACHED() and INJECTION_POINT(). The SQL functions injection_points_cached() and injection_points_run() are extended so as it is possible to pass an optional string to it. Reviewed-by: Greg Burd <g...@burd.me> Discussion: https://postgr.es/m/z_y9ttnxubvya...@paquier.xyz --- .../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..3da0cbc10e08 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, + void *arg); extern PGDLLEXPORT void injection_notice(const char *name, - const void *private_data); + const void *private_data, + void *arg); extern PGDLLEXPORT void injection_wait(const char *name, - const void *private_data); + const void *private_data, + 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, 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, 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, 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 ff370837a82c1b4a544005681ea61f30aae1e901 Mon Sep 17 00:00:00 2001 From: Michael Paquier <mich...@paquier.xyz> Date: Fri, 9 May 2025 15:42:36 +0900 Subject: [PATCH v3 3/3] aio: Use runtime arguments with injections points This cleans up some code related to the testing infrastructure of AIO based on injection points, switching the test code to use the newly-added facility in injection points rather than tweaks to pass arguments to the callbacks run. Reviewed-by: Greg Burd <g...@burd.me> Discussion: https://postgr.es/m/z_y9ttnxubvya...@paquier.xyz --- 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 04268e77ec2e..ebb5a771bfd1 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); @@ -1255,43 +1241,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 6e8b1327946e..743cccc2acd1 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 7745244b0e24..5cdfb89210b2 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, + void *arg); +extern PGDLLEXPORT void inj_io_reopen(const char *name, + const void *private_data, + 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, 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, void *arg) { ereport(LOG, errmsg("reopen injection point called, is enabled: %d", -- 2.49.0
signature.asc
Description: PGP signature
