public inbox for [email protected]help / color / mirror / Atom feed
injection_points: Switch wait/wakeup to use atomics rather than latches 21+ messages / 4 participants [nested] [flat]
* injection_points: Switch wait/wakeup to use atomics rather than latches @ 2026-05-28 02:43 Michael Paquier <[email protected]> 0 siblings, 1 reply; 21+ messages in thread From: Michael Paquier @ 2026-05-28 02:43 UTC (permalink / raw) To: Postgres hackers <[email protected]>; +Cc: Andrey Borodin <[email protected]> Hi all, (Adding Andrey in CC, as I'm sure he is interested in that.) While looking at the test proposed on the thread about the ProcKill(), I have been reminded about the fact that relying on latches and a condition variable for the wait and the wakeups has its limits: https://www.postgresql.org/message-id/aheVjCHmcbXBtiy0%40paquier.xyz In this case, we are trying to synchronize backends once they don't have latch assigned anymore, which defeats the purpose of wait/wakeup because the condition variable used in injection_points while waiting expects a Latch to be set for the processes we are waiting on. Folks have complained about this limitation a couple of times in the past, and I never got around to do something about it. While looking at that I have finished with the patch attached, which was surprisingly simpler than what I thought was needed. This replaces the condition variable with a set of atomic counters. The counters are incremented at wakeup, and the wait checks them on a periodic basis. The wait loop uses a delay that increases over time, maxed at 100ms so as we can get a good responsiveness on fast machines, without burning CPU for nothing in tests that require more wait time due to a tight loop with the counter checks. One thing worth noticing is the CHECK_FOR_INTERRUPTS() in the wait loop, which is something we need for the autovacuum test in test_misc that requires some signaling and interrupt processing. It may make sense to be conservative and limit ourselves to do this change on HEAD, but I'd like to suggest a backpatch down to v17 so as future tests that rely on a such change can be backpatched. I would need this change for the other test, still consistency in the facility primes for me here. Note: The CI seems happy with the patch. Thoughts or comments? -- Michael From 038b0f55dfe80f168fdc1b01b8cdadbf38fedfa2 Mon Sep 17 00:00:00 2001 From: Michael Paquier <[email protected]> Date: Thu, 28 May 2026 11:15:33 +0900 Subject: [PATCH] injection_points: Switch wait/wakeup to rely on atomics This removes the dependency based on counters and environment variables, replacing the waiting loop by a wait on an atomic counter, whose check increases over time in an exponential manner (starts at 10us, up to 100ms). --- .../injection_points/injection_points.c | 53 ++++++++++--------- 1 file changed, 27 insertions(+), 26 deletions(-) diff --git a/src/test/modules/injection_points/injection_points.c b/src/test/modules/injection_points/injection_points.c index ba282e3dcabf..9b8e1aaad0b0 100644 --- a/src/test/modules/injection_points/injection_points.c +++ b/src/test/modules/injection_points/injection_points.c @@ -23,11 +23,11 @@ #include "miscadmin.h" #include "nodes/pg_list.h" #include "nodes/value.h" -#include "storage/condition_variable.h" #include "storage/dsm_registry.h" #include "storage/ipc.h" #include "storage/lwlock.h" #include "storage/shmem.h" +#include "storage/spin.h" #include "utils/builtins.h" #include "utils/guc.h" #include "utils/injection_point.h" @@ -59,13 +59,10 @@ typedef struct InjectionPointSharedState slock_t lock; /* Counters advancing when injection_points_wakeup() is called */ - uint32 wait_counts[INJ_MAX_WAIT]; + pg_atomic_uint32 wait_counts[INJ_MAX_WAIT]; /* Names of injection points attached to wait counters */ char name[INJ_MAX_WAIT][INJ_NAME_MAXLEN]; - - /* Condition variable used for waits and wakeups */ - ConditionVariable wait_point; } InjectionPointSharedState; /* Pointer to shared-memory state. */ @@ -102,9 +99,9 @@ injection_point_init_state(void *ptr, void *arg) InjectionPointSharedState *state = (InjectionPointSharedState *) ptr; SpinLockInit(&state->lock); - memset(state->wait_counts, 0, sizeof(state->wait_counts)); memset(state->name, 0, sizeof(state->name)); - ConditionVariableInit(&state->wait_point); + for (int i = 0; i < INJ_MAX_WAIT; i++) + pg_atomic_init_u32(&state->wait_counts[i], 0); } static void @@ -222,7 +219,7 @@ injection_notice(const char *name, const void *private_data, void *arg) elog(NOTICE, "notice triggered for injection point %s", name); } -/* Wait on a condition variable, awaken by injection_points_wakeup() */ +/* Wait until injection_points_wakeup() is called */ void injection_wait(const char *name, const void *private_data, void *arg) { @@ -254,31 +251,37 @@ injection_wait(const char *name, const void *private_data, void *arg) { index = i; strlcpy(inj_state->name[i], name, INJ_NAME_MAXLEN); - old_wait_counts = inj_state->wait_counts[i]; + old_wait_counts = pg_atomic_read_u32(&inj_state->wait_counts[i]); break; } } SpinLockRelease(&inj_state->lock); if (index < 0) - elog(ERROR, "could not find free slot for wait of injection point %s ", + elog(ERROR, "could not find free slot for wait of injection point %s", name); - /* And sleep.. */ - ConditionVariablePrepareToSleep(&inj_state->wait_point); - for (;;) + /* + * Wait until the counter is bumped by injection_points_wakeup(). + * + * This loop starts with a short delay for responsiveness, enlarged to + * ease the CPU workload in slower environments. + */ +#define INJ_WAIT_INITIAL_US 10 /* 10us */ +#define INJ_WAIT_MAX_US 100000 /* 100ms */ + pgstat_report_wait_start(injection_wait_event); { - uint32 new_wait_counts; + int delay_us = INJ_WAIT_INITIAL_US; - SpinLockAcquire(&inj_state->lock); - new_wait_counts = inj_state->wait_counts[index]; - SpinLockRelease(&inj_state->lock); - - if (old_wait_counts != new_wait_counts) - break; - ConditionVariableSleep(&inj_state->wait_point, injection_wait_event); + while (pg_atomic_read_u32(&inj_state->wait_counts[index]) == old_wait_counts) + { + CHECK_FOR_INTERRUPTS(); + pg_usleep(delay_us); + if (delay_us < INJ_WAIT_MAX_US) + delay_us *= 2; + } } - ConditionVariableCancelSleep(); + pgstat_report_wait_end(); /* Remove this injection point from the waiters. */ SpinLockAcquire(&inj_state->lock); @@ -443,7 +446,7 @@ injection_points_wakeup(PG_FUNCTION_ARGS) if (inj_state == NULL) injection_init_shmem(); - /* First bump the wait counter for the injection point to wake up */ + /* Find the injection point then bump its wait counter */ SpinLockAcquire(&inj_state->lock); for (int i = 0; i < INJ_MAX_WAIT; i++) { @@ -458,11 +461,9 @@ injection_points_wakeup(PG_FUNCTION_ARGS) SpinLockRelease(&inj_state->lock); elog(ERROR, "could not find injection point %s to wake up", name); } - inj_state->wait_counts[index]++; SpinLockRelease(&inj_state->lock); - /* And broadcast the change to the waiters */ - ConditionVariableBroadcast(&inj_state->wait_point); + pg_atomic_fetch_add_u32(&inj_state->wait_counts[index], 1); PG_RETURN_VOID(); } -- 2.54.0 Attachments: [text/plain] 0001-injection_points-Switch-wait-wakeup-to-rely-on-atomi.patch (4.8K, ../../[email protected]/2-0001-injection_points-Switch-wait-wakeup-to-rely-on-atomi.patch) download | inline diff: From 038b0f55dfe80f168fdc1b01b8cdadbf38fedfa2 Mon Sep 17 00:00:00 2001 From: Michael Paquier <[email protected]> Date: Thu, 28 May 2026 11:15:33 +0900 Subject: [PATCH] injection_points: Switch wait/wakeup to rely on atomics This removes the dependency based on counters and environment variables, replacing the waiting loop by a wait on an atomic counter, whose check increases over time in an exponential manner (starts at 10us, up to 100ms). --- .../injection_points/injection_points.c | 53 ++++++++++--------- 1 file changed, 27 insertions(+), 26 deletions(-) diff --git a/src/test/modules/injection_points/injection_points.c b/src/test/modules/injection_points/injection_points.c index ba282e3dcabf..9b8e1aaad0b0 100644 --- a/src/test/modules/injection_points/injection_points.c +++ b/src/test/modules/injection_points/injection_points.c @@ -23,11 +23,11 @@ #include "miscadmin.h" #include "nodes/pg_list.h" #include "nodes/value.h" -#include "storage/condition_variable.h" #include "storage/dsm_registry.h" #include "storage/ipc.h" #include "storage/lwlock.h" #include "storage/shmem.h" +#include "storage/spin.h" #include "utils/builtins.h" #include "utils/guc.h" #include "utils/injection_point.h" @@ -59,13 +59,10 @@ typedef struct InjectionPointSharedState slock_t lock; /* Counters advancing when injection_points_wakeup() is called */ - uint32 wait_counts[INJ_MAX_WAIT]; + pg_atomic_uint32 wait_counts[INJ_MAX_WAIT]; /* Names of injection points attached to wait counters */ char name[INJ_MAX_WAIT][INJ_NAME_MAXLEN]; - - /* Condition variable used for waits and wakeups */ - ConditionVariable wait_point; } InjectionPointSharedState; /* Pointer to shared-memory state. */ @@ -102,9 +99,9 @@ injection_point_init_state(void *ptr, void *arg) InjectionPointSharedState *state = (InjectionPointSharedState *) ptr; SpinLockInit(&state->lock); - memset(state->wait_counts, 0, sizeof(state->wait_counts)); memset(state->name, 0, sizeof(state->name)); - ConditionVariableInit(&state->wait_point); + for (int i = 0; i < INJ_MAX_WAIT; i++) + pg_atomic_init_u32(&state->wait_counts[i], 0); } static void @@ -222,7 +219,7 @@ injection_notice(const char *name, const void *private_data, void *arg) elog(NOTICE, "notice triggered for injection point %s", name); } -/* Wait on a condition variable, awaken by injection_points_wakeup() */ +/* Wait until injection_points_wakeup() is called */ void injection_wait(const char *name, const void *private_data, void *arg) { @@ -254,31 +251,37 @@ injection_wait(const char *name, const void *private_data, void *arg) { index = i; strlcpy(inj_state->name[i], name, INJ_NAME_MAXLEN); - old_wait_counts = inj_state->wait_counts[i]; + old_wait_counts = pg_atomic_read_u32(&inj_state->wait_counts[i]); break; } } SpinLockRelease(&inj_state->lock); if (index < 0) - elog(ERROR, "could not find free slot for wait of injection point %s ", + elog(ERROR, "could not find free slot for wait of injection point %s", name); - /* And sleep.. */ - ConditionVariablePrepareToSleep(&inj_state->wait_point); - for (;;) + /* + * Wait until the counter is bumped by injection_points_wakeup(). + * + * This loop starts with a short delay for responsiveness, enlarged to + * ease the CPU workload in slower environments. + */ +#define INJ_WAIT_INITIAL_US 10 /* 10us */ +#define INJ_WAIT_MAX_US 100000 /* 100ms */ + pgstat_report_wait_start(injection_wait_event); { - uint32 new_wait_counts; + int delay_us = INJ_WAIT_INITIAL_US; - SpinLockAcquire(&inj_state->lock); - new_wait_counts = inj_state->wait_counts[index]; - SpinLockRelease(&inj_state->lock); - - if (old_wait_counts != new_wait_counts) - break; - ConditionVariableSleep(&inj_state->wait_point, injection_wait_event); + while (pg_atomic_read_u32(&inj_state->wait_counts[index]) == old_wait_counts) + { + CHECK_FOR_INTERRUPTS(); + pg_usleep(delay_us); + if (delay_us < INJ_WAIT_MAX_US) + delay_us *= 2; + } } - ConditionVariableCancelSleep(); + pgstat_report_wait_end(); /* Remove this injection point from the waiters. */ SpinLockAcquire(&inj_state->lock); @@ -443,7 +446,7 @@ injection_points_wakeup(PG_FUNCTION_ARGS) if (inj_state == NULL) injection_init_shmem(); - /* First bump the wait counter for the injection point to wake up */ + /* Find the injection point then bump its wait counter */ SpinLockAcquire(&inj_state->lock); for (int i = 0; i < INJ_MAX_WAIT; i++) { @@ -458,11 +461,9 @@ injection_points_wakeup(PG_FUNCTION_ARGS) SpinLockRelease(&inj_state->lock); elog(ERROR, "could not find injection point %s to wake up", name); } - inj_state->wait_counts[index]++; SpinLockRelease(&inj_state->lock); - /* And broadcast the change to the waiters */ - ConditionVariableBroadcast(&inj_state->wait_point); + pg_atomic_fetch_add_u32(&inj_state->wait_counts[index], 1); PG_RETURN_VOID(); } -- 2.54.0 [application/pgp-signature] signature.asc (833B, ../../[email protected]/3-signature.asc) download ^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: injection_points: Switch wait/wakeup to use atomics rather than latches @ 2026-05-28 12:40 Robert Haas <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 1 reply; 21+ messages in thread From: Robert Haas @ 2026-05-28 12:40 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Postgres hackers <[email protected]>; Andrey Borodin <[email protected]> On Wed, May 27, 2026 at 10:43 PM Michael Paquier <[email protected]> wrote: > While looking at the test proposed on the thread about the ProcKill(), > I have been reminded about the fact that relying on latches and a > condition variable for the wait and the wakeups has its limits: > https://www.postgresql.org/message-id/aheVjCHmcbXBtiy0%40paquier.xyz After reading this email, the linked-to email, and the commit message for the patch, I still don't have a clear understanding of what this is intended to fix. It seems like it's going to make the responsiveness worse. In general, we want to replace escalating wait loops with things that wake up instantly at the right time, and this is going in the opposite direction. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: injection_points: Switch wait/wakeup to use atomics rather than latches @ 2026-05-28 23:19 Michael Paquier <[email protected]> parent: Robert Haas <[email protected]> 0 siblings, 1 reply; 21+ messages in thread From: Michael Paquier @ 2026-05-28 23:19 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: Postgres hackers <[email protected]>; Andrey Borodin <[email protected]> On Thu, May 28, 2026 at 08:40:39AM -0400, Robert Haas wrote: > After reading this email, the linked-to email, and the commit message > for the patch, I still don't have a clear understanding of what this > is intended to fix. It seems like it's going to make the > responsiveness worse. In general, we want to replace escalating wait > loops with things that wake up instantly at the right time, and this > is going in the opposite direction. This is an exchange between responsiveness of the system and flexibility. I have had two complaints in the past about the fact that the waits and wakeups were not doable due to the fact that we rely on condition variables and latches: - Postmaster context (lack of dsm access as one). Heikki has mentioned that to me once as annoying when hacking on tests there at protocol level, at least. - Second case as shown on the previous thread, which was a tricky scenario involving the termination of backends. One limitation is also related to wait event visibility, which may not be visible in pg_stat_activity. We could simply add a LOG entry in injection_wait() once the old count is read, and rely on a server log lookup in the TAP tests where we cannot use pg_stat_activity. Compared to redesigning all the facilities that injection_points relies on, this patch was striking me as having a good balance in terms of responsiveness (min 10us, max 100ms) vs portability. The minimum threshold does not really matter much in terms of runtime on fast machines. Does this explanation make sense? -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: injection_points: Switch wait/wakeup to use atomics rather than latches @ 2026-05-29 12:48 Robert Haas <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 1 reply; 21+ messages in thread From: Robert Haas @ 2026-05-29 12:48 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Postgres hackers <[email protected]>; Andrey Borodin <[email protected]> On Thu, May 28, 2026 at 7:19 PM Michael Paquier <[email protected]> wrote: > On Thu, May 28, 2026 at 08:40:39AM -0400, Robert Haas wrote: > > After reading this email, the linked-to email, and the commit message > > for the patch, I still don't have a clear understanding of what this > > is intended to fix. It seems like it's going to make the > > responsiveness worse. In general, we want to replace escalating wait > > loops with things that wake up instantly at the right time, and this > > is going in the opposite direction. > > This is an exchange between responsiveness of the system and > flexibility. I have had two complaints in the past about the fact > that the waits and wakeups were not doable due to the fact that we > rely on condition variables and latches: I'm still struggling to understand. Condition variables and latches are both designed to allow for nice waits and wakeups. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: injection_points: Switch wait/wakeup to use atomics rather than latches @ 2026-05-29 13:31 Heikki Linnakangas <[email protected]> parent: Robert Haas <[email protected]> 0 siblings, 2 replies; 21+ messages in thread From: Heikki Linnakangas @ 2026-05-29 13:31 UTC (permalink / raw) To: Robert Haas <[email protected]>; Michael Paquier <[email protected]>; +Cc: Postgres hackers <[email protected]>; Andrey Borodin <[email protected]> On 29/05/2026 15:48, Robert Haas wrote: > On Thu, May 28, 2026 at 7:19 PM Michael Paquier <[email protected]> wrote: >> On Thu, May 28, 2026 at 08:40:39AM -0400, Robert Haas wrote: >>> After reading this email, the linked-to email, and the commit message >>> for the patch, I still don't have a clear understanding of what this >>> is intended to fix. It seems like it's going to make the >>> responsiveness worse. In general, we want to replace escalating wait >>> loops with things that wake up instantly at the right time, and this >>> is going in the opposite direction. >> >> This is an exchange between responsiveness of the system and >> flexibility. I have had two complaints in the past about the fact >> that the waits and wakeups were not doable due to the fact that we >> rely on condition variables and latches: > > I'm still struggling to understand. Condition variables and latches > are both designed to allow for nice waits and wakeups. They only work after you have a PGPROC slot. If you want to inject code to authentication, or into postmaster, you cannot use them. - Heikki ^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: injection_points: Switch wait/wakeup to use atomics rather than latches @ 2026-05-29 16:00 Robert Haas <[email protected]> parent: Heikki Linnakangas <[email protected]> 1 sibling, 1 reply; 21+ messages in thread From: Robert Haas @ 2026-05-29 16:00 UTC (permalink / raw) To: Heikki Linnakangas <[email protected]>; +Cc: Michael Paquier <[email protected]>; Postgres hackers <[email protected]>; Andrey Borodin <[email protected]> On Fri, May 29, 2026 at 9:31 AM Heikki Linnakangas <[email protected]> wrote: > They only work after you have a PGPROC slot. If you want to inject code > to authentication, or into postmaster, you cannot use them. OK, got it now. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: injection_points: Switch wait/wakeup to use atomics rather than latches @ 2026-05-30 04:13 Michael Paquier <[email protected]> parent: Robert Haas <[email protected]> 0 siblings, 0 replies; 21+ messages in thread From: Michael Paquier @ 2026-05-30 04:13 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: Heikki Linnakangas <[email protected]>; Postgres hackers <[email protected]>; Andrey Borodin <[email protected]> On Fri, May 29, 2026 at 12:00:46PM -0400, Robert Haas wrote: > On Fri, May 29, 2026 at 9:31 AM Heikki Linnakangas <[email protected]> wrote: >> They only work after you have a PGPROC slot. If you want to inject code >> to authentication, or into postmaster, you cannot use them. > > OK, got it now. It seems like Heikki's comment was better worded than mine. Also mentioned upthread, but the lack of PGPROC also means a lack of monitoring as wait events cannot be tracked. Currently, we rely on that in the TAP tests. For cases where the procs are not available, I don't have a better idea than generating a LOG entry after the wait counts have been generated (with a PID) and couple that with a poll of the server logs to let a script understand that a process is in waiting mode. As a whole, I don't think that we should try to be fancy with the implementation, which is why I have used primitives that should work in any context, and I'm not convinced that this is worth its own facility if that just means more responsiveness (in most cases the wait should not take more than a couple ms to notice a wakeup). I'm open to more fancy ideas, of course. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: injection_points: Switch wait/wakeup to use atomics rather than latches @ 2026-05-30 08:05 Andrey Borodin <[email protected]> parent: Heikki Linnakangas <[email protected]> 1 sibling, 1 reply; 21+ messages in thread From: Andrey Borodin @ 2026-05-30 08:05 UTC (permalink / raw) To: Heikki Linnakangas <[email protected]>; +Cc: Robert Haas <[email protected]>; Michael Paquier <[email protected]>; Postgres hackers <[email protected]> > On 28 May 2026, at 07:43, Michael Paquier <[email protected]> wrote: > > Andrey in CC, as I'm sure he is interested in that. Thanks! That's exactly what I need for my tests. > On 29 May 2026, at 18:31, Heikki Linnakangas <[email protected]> wrote: > >> I'm still struggling to understand. Condition variables and latches >> are both designed to allow for nice waits and wakeups. > > They only work after you have a PGPROC slot. If you want to inject code to authentication, or into postmaster, you cannot use them. I have another reason: postmaster death behavior. When we wait on ConVar and postmaster is kill-9-ed, we release all LWLocks. Which causes corruption [0], because checkpointer can flush something that's not in WAL. So I'm trying to build corruption-seeking tests using tool that can induce corruption in tests. About the patch: - inj_state->wait_counts[index]++; SpinLockRelease(&inj_state->lock); - /* And broadcast the change to the waiters */ - ConditionVariableBroadcast(&inj_state->wait_point); + pg_atomic_fetch_add_u32(&inj_state->wait_counts[index], 1); Can we move pg_atomic_fetch_add_u32() back under the lock? We determine slot index under lock, then wakeup slot outside the lock. In a correctly written test meaning this is not a problem. However, technically, identity of a slot can change between releasing the lock and incrementing wait_counts[index]. I'll do another pass tomorrow, maybe something else will catch my eye. Best regards, Andrey Borodin. [0] https://www.postgresql.org/message-id/B3C69B86-7F82-4111-B97F-0005497BB745%40yandex-team.ru ^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: injection_points: Switch wait/wakeup to use atomics rather than latches @ 2026-06-01 11:25 Andrey Borodin <[email protected]> parent: Andrey Borodin <[email protected]> 0 siblings, 1 reply; 21+ messages in thread From: Andrey Borodin @ 2026-06-01 11:25 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Robert Haas <[email protected]>; Postgres hackers <[email protected]>; Heikki Linnakangas <[email protected]> > On 30 May 2026, at 13:05, Andrey Borodin <[email protected]> wrote: > > I'll do another pass tomorrow, maybe something else will catch my eye. I've tried the patch on my old corruption experiments. And it works for me. I had to switch killing to something like foreach my $i (1 .. 100) { my @alive = grep { kill 0, $_ } @cluster_pids; last unless @alive; kill 'KILL', @alive; usleep(100_000); } The shared memory segment must be released before we attempt recovery. But that's exactly what I wanted anyway. Thank you! Best regards, Andrey Borodin. ^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: injection_points: Switch wait/wakeup to use atomics rather than latches @ 2026-06-02 04:15 Michael Paquier <[email protected]> parent: Andrey Borodin <[email protected]> 0 siblings, 1 reply; 21+ messages in thread From: Michael Paquier @ 2026-06-02 04:15 UTC (permalink / raw) To: Andrey Borodin <[email protected]>; +Cc: Robert Haas <[email protected]>; Postgres hackers <[email protected]>; Heikki Linnakangas <[email protected]> On Mon, Jun 01, 2026 at 04:25:40PM +0500, Andrey Borodin wrote: > The shared memory segment must be released before we attempt recovery. > But that's exactly what I wanted anyway. Thank you! How do you guarantee that a wait position is reached in the case where you don't have access to wait events to make sure that the wait point is reached? -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: injection_points: Switch wait/wakeup to use atomics rather than latches @ 2026-06-02 05:13 Andrey Borodin <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 1 reply; 21+ messages in thread From: Andrey Borodin @ 2026-06-02 05:13 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Robert Haas <[email protected]>; Postgres hackers <[email protected]>; Heikki Linnakangas <[email protected]> > On 2 Jun 2026, at 09:15, Michael Paquier <[email protected]> wrote: > > On Mon, Jun 01, 2026 at 04:25:40PM +0500, Andrey Borodin wrote: >> The shared memory segment must be released before we attempt recovery. >> But that's exactly what I wanted anyway. Thank you! > > How do you guarantee that a wait position is reached in the case where > you don't have access to wait events to make sure that the wait point > is reached? In a research test? sleep(1) Best regards, Andrey Borodin. ^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: injection_points: Switch wait/wakeup to use atomics rather than latches @ 2026-06-02 05:27 Michael Paquier <[email protected]> parent: Andrey Borodin <[email protected]> 0 siblings, 1 reply; 21+ messages in thread From: Michael Paquier @ 2026-06-02 05:27 UTC (permalink / raw) To: Andrey Borodin <[email protected]>; +Cc: Robert Haas <[email protected]>; Postgres hackers <[email protected]>; Heikki Linnakangas <[email protected]> On Tue, Jun 02, 2026 at 10:13:01AM +0500, Andrey Borodin wrote: > In a research test? sleep(1) A hardcoded sleep can work on a fast machine but it makes the test slow. A sleep is not a reliable technique if running the tests on a slow machine, as an expected wait point may not have been reached. We have both very slow and very fast animals in the buildfarm. Rewording my question a bit: did you consider some options regarding what an equivalent of a wait event lookup should look like when we don't have a PGPROC? My idea of printing a LOG and do a server log lookup would work, just asking if others have better ideas than the only one I got. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: injection_points: Switch wait/wakeup to use atomics rather than latches @ 2026-06-02 18:46 Andrey Borodin <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 1 reply; 21+ messages in thread From: Andrey Borodin @ 2026-06-02 18:46 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Robert Haas <[email protected]>; Postgres hackers <[email protected]>; Heikki Linnakangas <[email protected]> > On 2 Jun 2026, at 10:27, Michael Paquier <[email protected]> wrote: > > Rewording my question a bit: did you consider some options regarding > what an equivalent of a wait event lookup should look like when we > don't have a PGPROC? My idea of printing a LOG and do a server log > lookup would work, just asking if others have better ideas than the > only one I got. For tests without PGPROC we can mmap() inj_state to a fixed file in PGDATA/injection_points.shm. TAP can poll name[] to detect that a wait point was reached and bump wait_counts[] to wake. Best regards, Andrey Borodin. ^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: injection_points: Switch wait/wakeup to use atomics rather than latches @ 2026-06-02 22:23 Michael Paquier <[email protected]> parent: Andrey Borodin <[email protected]> 0 siblings, 1 reply; 21+ messages in thread From: Michael Paquier @ 2026-06-02 22:23 UTC (permalink / raw) To: Andrey Borodin <[email protected]>; +Cc: Robert Haas <[email protected]>; Postgres hackers <[email protected]>; Heikki Linnakangas <[email protected]> On Tue, Jun 02, 2026 at 11:46:52PM +0500, Andrey Borodin wrote: > For tests without PGPROC we can mmap() inj_state to a fixed file in > PGDATA/injection_points.shm. TAP can poll name[] to detect that a wait > point was reached and bump wait_counts[] to wake. That's a direction. Only mmap() would not be sufficient, as WIN32 has its own non-POSIX idea on the matter with CreateFileMapping() & friends. I am wondering if we should think harder about an interface that could make such things easier for extensions. Or we could have a portable layer added to injection_points, as well.. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: injection_points: Switch wait/wakeup to use atomics rather than latches @ 2026-06-12 07:02 Andrey Borodin <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 1 reply; 21+ messages in thread From: Andrey Borodin @ 2026-06-12 07:02 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Robert Haas <[email protected]>; Postgres hackers <[email protected]>; Heikki Linnakangas <[email protected]> Hi Michael, > On 3 Jun 2026, at 03:23, Michael Paquier <[email protected]> wrote: > > That's a direction. Only mmap() would not be sufficient, as WIN32 has > its own non-POSIX idea on the matter with CreateFileMapping() & > friends. Right, mmap() alone is not enough. I hacked up a prototype (on top of your atomics commit) that maps the state portably on both sides: POSIX mmap() in the backend and a file-backed CreateFileMapping() on WIN32, and the same in a small standalone client. Perl cannot mmap() portably, so the mapping is done by a tiny C helper that TAP drives, rather than from the test script itself. > I am wondering if we should think harder about an interface that could > make such things easier for extensions. Or we could have a portable > layer added to injection_points, as well.. While prototyping I hit a constraint that I think answers this. To arm a point without SQL it is not enough to reach this module's wait state: you have to reach the registry of active points, i.e. ActiveInjectionPoints in injection_point.c. INJECTION_POINT() consults that array, and the module only supplies the callback once the core has found the name. So the part that has to be reachable from outside lives in the core, not in the module. That splits the problem into two layers rather cleanly: - core: the active-points array is backed by a file (injection_points.shm in the data directory). This is the generic piece - any extension could arm or inspect points out of band, and it is what makes attach-without-SQL possible. The lock-free generation protocol that reads the array is unchanged, so external readers can rely on it too. - module: injection_points keeps its own file (injection_points_wait.shm) for the wait/wakeup coordination of the "wait" action. That part is test-specific and stays out of the core. A standalone client (injection_points_state) maps both files the way the backend does and can attach/detach a point and detect+release a waiter, with no backend connection and no PGPROC. A TAP test runs the whole flow without SQL for arming or waking: arm a wait point with the client, trigger it from a background session, let the client poll the mapped file until the point is reached (so no sleep, and it behaves the same on slow and fast animals), disarm it, then wake. SQL is used only to trigger the point and to observe injection_points_list(). This is a rough prototype to make the discussion concrete, not a proposal of the final shape. To name few open points: 1. Where the portable mapping helper should live. 1. Keep the create-or-attach-file helper (POSIX + WIN32) local to injection_point.c, as in the prototype. Simplest, no new public surface. 2. Factor a small portable "named file-backed shared region" that any extension could use - an "interface for extensions". More general, but a larger commitment and more to review. I have a slight preference for 1 now, growing into 2 only once a second user actually appears. 2. One file or two. The prototype keeps the core registry and the module's wait coordination in two files. Folding a wait counter into the core InjectionPointEntry would make it a single file, but it pushes test-only wait semantics into the core struct, which I would rather avoid. Slight preference for two files. 3. The client writes the registry locklessly (no InjectionPointLock), so it assumes nothing else attaches/detaches the same array concurrently. That holds for arming points out of band around a controlled session, but it is not a general concurrent-safe writer. I think that is acceptable for tests, but I am not sure. 4. The path defaults to injection_points.shm under the data directory, with an env override (PG_INJECTION_POINTS_FILE) so a point could be armed before the data dir exists or in single player mode. I have not added a test for that, so probably it does not work. Yet. 5. The external mirror reads and writes the pg_atomic_uint{32,64} fields as plain integers. That only holds where those atomics are a bare value; on a platform without native 64-bit atomics pg_atomic_uint64 falls back to a spinlock-protected struct with a different layout, so the byte mirror would not match (and the backend's width assertion would fail to build there in the first place). Even where they are native, the 64-bit field forces 8-byte alignment that the mirror must replicate - I got bitten by exactly that on the 32-bit build, where the entries array otherwise starts 4 bytes too early. So a robust portable layer, if we want one, is not as trivial as it first looked to me. 6. Whether the external interface should do more than "wait". The file-backed registry already lets an outside process attach a point to any callback (error, notice, ...); the only thing we can observe back without SQL today is that a wait was reached. A natural generalization is a per-point hit counter that any process bumps when it runs the point, readable from outside. That would let a test assert "initdb really went through this path", or "the postmaster reached this point before it failed at startup" - cases where there is no backend to query. I have not built it, and it leans the same way as 2. (it would put an observation counter into the core entry), but it seems like the obvious next use-case if we go this route. WDYT? I am not at all sure the file-backed registry is the right long-term shape; if you prefer the elog-and-grep route I am happy to drop this, it was mostly a way to see how invasive the alternative really is. Thank you! Best regards, Andrey Borodin. P.S. Fun observation - it speeds up tests. I swapped the "point reached" detection in test_misc/010_index_concurrently_upsert.pl (41 cases, each polling pg_stat_activity every 100ms via a fresh psql) for a 10ms poll of the mapped file through the client. Median test wall time dropped from 2.2s to 1.6s (~27%) on my MacBook Air M5. Attachments: [application/octet-stream] v2026-06-12-0001-injection_points-Switch-wait-wakeup-to-r.patch (4.9K, ../../[email protected]/2-v2026-06-12-0001-injection_points-Switch-wait-wakeup-to-r.patch) download | inline diff: From 80cb6d9463194d887fc88a3567a82e8322b5f6c1 Mon Sep 17 00:00:00 2001 From: Michael Paquier <[email protected]> Date: Thu, 28 May 2026 11:15:33 +0900 Subject: [PATCH v2026-06-12 1/3] injection_points: Switch wait/wakeup to rely on atomics This removes the dependency based on counters and environment variables, replacing the waiting loop by a wait on an atomic counter, whose check increases over time in an exponential manner (starts at 10us, up to 100ms). --- .../injection_points/injection_points.c | 53 ++++++++++--------- 1 file changed, 27 insertions(+), 26 deletions(-) diff --git a/src/test/modules/injection_points/injection_points.c b/src/test/modules/injection_points/injection_points.c index ba282e3dcab..9b8e1aaad0b 100644 --- a/src/test/modules/injection_points/injection_points.c +++ b/src/test/modules/injection_points/injection_points.c @@ -23,11 +23,11 @@ #include "miscadmin.h" #include "nodes/pg_list.h" #include "nodes/value.h" -#include "storage/condition_variable.h" #include "storage/dsm_registry.h" #include "storage/ipc.h" #include "storage/lwlock.h" #include "storage/shmem.h" +#include "storage/spin.h" #include "utils/builtins.h" #include "utils/guc.h" #include "utils/injection_point.h" @@ -59,13 +59,10 @@ typedef struct InjectionPointSharedState slock_t lock; /* Counters advancing when injection_points_wakeup() is called */ - uint32 wait_counts[INJ_MAX_WAIT]; + pg_atomic_uint32 wait_counts[INJ_MAX_WAIT]; /* Names of injection points attached to wait counters */ char name[INJ_MAX_WAIT][INJ_NAME_MAXLEN]; - - /* Condition variable used for waits and wakeups */ - ConditionVariable wait_point; } InjectionPointSharedState; /* Pointer to shared-memory state. */ @@ -102,9 +99,9 @@ injection_point_init_state(void *ptr, void *arg) InjectionPointSharedState *state = (InjectionPointSharedState *) ptr; SpinLockInit(&state->lock); - memset(state->wait_counts, 0, sizeof(state->wait_counts)); memset(state->name, 0, sizeof(state->name)); - ConditionVariableInit(&state->wait_point); + for (int i = 0; i < INJ_MAX_WAIT; i++) + pg_atomic_init_u32(&state->wait_counts[i], 0); } static void @@ -222,7 +219,7 @@ injection_notice(const char *name, const void *private_data, void *arg) elog(NOTICE, "notice triggered for injection point %s", name); } -/* Wait on a condition variable, awaken by injection_points_wakeup() */ +/* Wait until injection_points_wakeup() is called */ void injection_wait(const char *name, const void *private_data, void *arg) { @@ -254,31 +251,37 @@ injection_wait(const char *name, const void *private_data, void *arg) { index = i; strlcpy(inj_state->name[i], name, INJ_NAME_MAXLEN); - old_wait_counts = inj_state->wait_counts[i]; + old_wait_counts = pg_atomic_read_u32(&inj_state->wait_counts[i]); break; } } SpinLockRelease(&inj_state->lock); if (index < 0) - elog(ERROR, "could not find free slot for wait of injection point %s ", + elog(ERROR, "could not find free slot for wait of injection point %s", name); - /* And sleep.. */ - ConditionVariablePrepareToSleep(&inj_state->wait_point); - for (;;) + /* + * Wait until the counter is bumped by injection_points_wakeup(). + * + * This loop starts with a short delay for responsiveness, enlarged to + * ease the CPU workload in slower environments. + */ +#define INJ_WAIT_INITIAL_US 10 /* 10us */ +#define INJ_WAIT_MAX_US 100000 /* 100ms */ + pgstat_report_wait_start(injection_wait_event); { - uint32 new_wait_counts; + int delay_us = INJ_WAIT_INITIAL_US; - SpinLockAcquire(&inj_state->lock); - new_wait_counts = inj_state->wait_counts[index]; - SpinLockRelease(&inj_state->lock); - - if (old_wait_counts != new_wait_counts) - break; - ConditionVariableSleep(&inj_state->wait_point, injection_wait_event); + while (pg_atomic_read_u32(&inj_state->wait_counts[index]) == old_wait_counts) + { + CHECK_FOR_INTERRUPTS(); + pg_usleep(delay_us); + if (delay_us < INJ_WAIT_MAX_US) + delay_us *= 2; + } } - ConditionVariableCancelSleep(); + pgstat_report_wait_end(); /* Remove this injection point from the waiters. */ SpinLockAcquire(&inj_state->lock); @@ -443,7 +446,7 @@ injection_points_wakeup(PG_FUNCTION_ARGS) if (inj_state == NULL) injection_init_shmem(); - /* First bump the wait counter for the injection point to wake up */ + /* Find the injection point then bump its wait counter */ SpinLockAcquire(&inj_state->lock); for (int i = 0; i < INJ_MAX_WAIT; i++) { @@ -458,11 +461,9 @@ injection_points_wakeup(PG_FUNCTION_ARGS) SpinLockRelease(&inj_state->lock); elog(ERROR, "could not find injection point %s to wake up", name); } - inj_state->wait_counts[index]++; SpinLockRelease(&inj_state->lock); - /* And broadcast the change to the waiters */ - ConditionVariableBroadcast(&inj_state->wait_point); + pg_atomic_fetch_add_u32(&inj_state->wait_counts[index], 1); PG_RETURN_VOID(); } -- 2.50.1 (Apple Git-155) [application/octet-stream] v2026-06-12-0003-injection_points-attach-and-coordinate-w.patch (34.9K, ../../[email protected]/3-v2026-06-12-0003-injection_points-attach-and-coordinate-w.patch) download | inline diff: From 174f0e288dd1e11c31550ff06bac8c6ddb541803 Mon Sep 17 00:00:00 2001 From: Andrey Borodin <[email protected]> Date: Fri, 12 Jun 2026 10:38:21 +0500 Subject: [PATCH v2026-06-12 3/3] injection_points: attach and coordinate wait points without SQL Move this module's wait/wakeup state out of a DSM into its own file (injection_points_wait.shm) and add injection_points_state, a small client that maps both files the way the backend does to attach/detach a wait point and to detect and release a waiter, all without SQL. A TAP test exercises the no-SQL flow. --- src/test/modules/injection_points/.gitignore | 3 + src/test/modules/injection_points/Makefile | 21 + .../injection_points/injection_points.c | 245 ++++++++-- .../injection_points/injection_points.h | 39 ++ .../injection_points/injection_points_state.c | 460 ++++++++++++++++++ src/test/modules/injection_points/meson.build | 21 + .../t/001_wait_without_sql.pl | 128 +++++ 7 files changed, 881 insertions(+), 36 deletions(-) create mode 100644 src/test/modules/injection_points/injection_points_state.c create mode 100644 src/test/modules/injection_points/t/001_wait_without_sql.pl diff --git a/src/test/modules/injection_points/.gitignore b/src/test/modules/injection_points/.gitignore index 0de307e70a6..88fccf29987 100644 --- a/src/test/modules/injection_points/.gitignore +++ b/src/test/modules/injection_points/.gitignore @@ -4,3 +4,6 @@ /results/ /tmp_check/ /tmp_check_iso/ + +# Standalone state client +/injection_points_state diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile index c01d2fb095c..b7387abe84c 100644 --- a/src/test/modules/injection_points/Makefile +++ b/src/test/modules/injection_points/Makefile @@ -24,10 +24,19 @@ ISOLATION = basic \ # some isolation tests require wal_level=replica ISOLATION_OPTS = --temp-config $(top_srcdir)/src/test/modules/injection_points/extra.conf +TAP_TESTS = 1 + # The injection points are cluster-wide, so disable installcheck NO_INSTALLCHECK = 1 +# Standalone client used by the TAP tests to wait on and wake up injection +# points without a backend connection. It is built alongside the module +# (MODULE_big already owns OBJS, so it cannot go through PGXS PROGRAM) and its +# absolute path is exported for the TAP tests. +INJ_STATE_CLIENT = injection_points_state$(X) + export enable_injection_points +export INJECTION_POINTS_STATE := $(abspath $(INJ_STATE_CLIENT)) ifdef USE_PGXS PG_CONFIG = pg_config @@ -51,3 +60,15 @@ check: endif endif + +# Build the standalone state client. Its object is compiled by the implicit +# rule (which adds -I. so injection_points.h is found) and it links against no +# backend libraries. +all: $(INJ_STATE_CLIENT) + +$(INJ_STATE_CLIENT): injection_points_state.o + $(CC) $(CFLAGS) $< $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@ + +clean: clean-injection-points-state +clean-injection-points-state: + rm -f $(INJ_STATE_CLIENT) injection_points_state.o diff --git a/src/test/modules/injection_points/injection_points.c b/src/test/modules/injection_points/injection_points.c index 9b8e1aaad0b..a868e3d2ac5 100644 --- a/src/test/modules/injection_points/injection_points.c +++ b/src/test/modules/injection_points/injection_points.c @@ -17,13 +17,18 @@ #include "postgres.h" +#include <fcntl.h> +#ifndef WIN32 +#include <sys/mman.h> +#endif + #include "fmgr.h" #include "funcapi.h" #include "injection_points.h" #include "miscadmin.h" #include "nodes/pg_list.h" #include "nodes/value.h" -#include "storage/dsm_registry.h" +#include "storage/fd.h" #include "storage/ipc.h" #include "storage/lwlock.h" #include "storage/shmem.h" @@ -37,10 +42,6 @@ PG_MODULE_MAGIC; -/* Maximum number of waits usable in injection points at once */ -#define INJ_MAX_WAIT 8 -#define INJ_NAME_MAXLEN 64 - /* * List of injection points stored in TopMemoryContext attached * locally to this process. @@ -50,22 +51,40 @@ static List *inj_list_local = NIL; /* * Shared state information for injection points. * - * This state data can be initialized in two ways: dynamically with a DSM - * or when loading the module. + * This is mapped from a fixed file in the data directory (INJ_STATE_FILE) + * rather than being allocated in the main shared memory segment or a DSM. + * Backing it with a file lets external programs without a backend connection + * map the same state, observe which injection points are being waited on and + * release them (see injection_points_state.c). This works in contexts where + * condition variables and latches are unavailable, e.g. the postmaster or a + * process that has not set up its PGPROC yet. + * + * The leading InjectionPointPublicState portion is the contract shared with + * those external tools, so it must stay first and keep a frontend-compatible + * layout (see injection_points.h). The fields below it are backend-only. */ typedef struct InjectionPointSharedState { - /* Protects access to other fields */ - slock_t lock; + /* Names of injection points attached to wait counters (slot in use). */ + char name[INJ_MAX_WAIT][INJ_NAME_MAXLEN]; /* Counters advancing when injection_points_wakeup() is called */ pg_atomic_uint32 wait_counts[INJ_MAX_WAIT]; - /* Names of injection points attached to wait counters */ - char name[INJ_MAX_WAIT][INJ_NAME_MAXLEN]; + /* Protects access to the name array (backend-only) */ + slock_t lock; } InjectionPointSharedState; -/* Pointer to shared-memory state. */ +/* The public prefix must match InjectionPointPublicState bit for bit. */ +StaticAssertDecl(offsetof(InjectionPointSharedState, name) == 0, + "name must be the first field of InjectionPointSharedState"); +StaticAssertDecl(offsetof(InjectionPointSharedState, wait_counts) == + offsetof(InjectionPointPublicState, wait_counts), + "wait_counts offset must match InjectionPointPublicState"); +StaticAssertDecl(sizeof(pg_atomic_uint32) == sizeof(uint32), + "pg_atomic_uint32 must be layout-compatible with uint32"); + +/* Pointer to the mapped shared state. */ static InjectionPointSharedState *inj_state = NULL; extern PGDLLEXPORT void injection_error(const char *name, @@ -81,63 +100,217 @@ 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; -static void injection_shmem_request(void *arg); +/* How injection_map_state() should open the backing file. */ +typedef enum InjectionMapMode +{ + INJ_MAP_CREATE, /* discard any stale file, create fresh */ + INJ_MAP_ATTACH, /* map an already-existing file */ + INJ_MAP_ATTACH_OR_CREATE, /* attach if present, else create */ +} InjectionMapMode; + static void injection_shmem_init(void *arg); +static void injection_shmem_attach(void *arg); static const ShmemCallbacks injection_shmem_callbacks = { - .request_fn = injection_shmem_request, + /* Create and initialize the backing file once at postmaster startup. */ .init_fn = injection_shmem_init, + /* Re-map it in each child that does not inherit the mapping (Windows). */ + .attach_fn = injection_shmem_attach, }; /* - * Routine for shared memory area initialization, used as a callback - * when initializing dynamically with a DSM or when loading the module. + * Initialize a freshly-created shared state. */ static void -injection_point_init_state(void *ptr, void *arg) +injection_point_init_state(InjectionPointSharedState *state) { - InjectionPointSharedState *state = (InjectionPointSharedState *) ptr; - SpinLockInit(&state->lock); memset(state->name, 0, sizeof(state->name)); for (int i = 0; i < INJ_MAX_WAIT; i++) pg_atomic_init_u32(&state->wait_counts[i], 0); } +/* + * proc_exit callback removing the backing file. Registered only by the + * process that created it (the postmaster, or a lone backend when the module + * is not preloaded), so that the file disappears together with the cluster. + */ +static void +injection_state_file_cleanup(int code, Datum arg) +{ + if (inj_state != NULL) + { +#ifndef WIN32 + munmap(inj_state, sizeof(InjectionPointSharedState)); +#else + UnmapViewOfFile(inj_state); +#endif + inj_state = NULL; + } + + /* + * Only the postmaster (or a standalone backend) should unlink the file; + * forked children inherit this callback but must not remove it. + */ + if (!IsUnderPostmaster) + (void) unlink(INJ_STATE_FILE); +} + +/* + * Map INJ_STATE_FILE into this process, creating and/or initializing it as + * dictated by "mode", and set inj_state. + * + * The state is backed by an ordinary file so that external programs can map + * the same bytes (POSIX mmap or, on Windows, a file-backed CreateFileMapping). + * All accessors must use the mapping, never plain file reads, because file + * I/O is not guaranteed to be coherent with a mapped view on Windows. + */ static void -injection_shmem_request(void *arg) +injection_map_state(InjectionMapMode mode, int elevel) { - ShmemRequestStruct(.name = "injection_points", - .size = sizeof(InjectionPointSharedState), - .ptr = (void **) &inj_state, - ); + Size size = sizeof(InjectionPointSharedState); + bool created = false; + + if (inj_state != NULL) + return; + + if (mode == INJ_MAP_CREATE) + (void) unlink(INJ_STATE_FILE); /* drop any stale file from a crash */ + +#ifndef WIN32 + { + int fd; + int oflags = O_RDWR; + + if (mode != INJ_MAP_ATTACH) + oflags |= O_CREAT | O_EXCL; + + fd = OpenTransientFile(INJ_STATE_FILE, oflags); + if (fd < 0 && mode == INJ_MAP_ATTACH_OR_CREATE && errno == EEXIST) + { + /* Lost the race to create it; just attach. */ + oflags = O_RDWR; + fd = OpenTransientFile(INJ_STATE_FILE, oflags); + } + else if (fd >= 0 && (oflags & O_CREAT)) + created = true; + + if (fd < 0) + ereport(elevel, + (errcode_for_file_access(), + errmsg("could not open injection point state file \"%s\": %m", + INJ_STATE_FILE))); + + if (created && ftruncate(fd, size) != 0) + { + CloseTransientFile(fd); + (void) unlink(INJ_STATE_FILE); + ereport(elevel, + (errcode_for_file_access(), + errmsg("could not size injection point state file \"%s\": %m", + INJ_STATE_FILE))); + } + + inj_state = mmap(NULL, size, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0); + CloseTransientFile(fd); + + if (inj_state == MAP_FAILED) + { + inj_state = NULL; + ereport(elevel, + (errcode_for_file_access(), + errmsg("could not map injection point state file \"%s\": %m", + INJ_STATE_FILE))); + } + } +#else + { + HANDLE hfile; + HANDLE hmap; + DWORD disp = (mode == INJ_MAP_ATTACH) ? OPEN_EXISTING : CREATE_NEW; + + hfile = CreateFile(INJ_STATE_FILE, + GENERIC_READ | GENERIC_WRITE, + FILE_SHARE_READ | FILE_SHARE_WRITE, + NULL, disp, FILE_ATTRIBUTE_NORMAL, NULL); + if (hfile == INVALID_HANDLE_VALUE && + mode == INJ_MAP_ATTACH_OR_CREATE && + GetLastError() == ERROR_FILE_EXISTS) + { + disp = OPEN_EXISTING; + hfile = CreateFile(INJ_STATE_FILE, + GENERIC_READ | GENERIC_WRITE, + FILE_SHARE_READ | FILE_SHARE_WRITE, + NULL, disp, FILE_ATTRIBUTE_NORMAL, NULL); + } + else if (hfile != INVALID_HANDLE_VALUE && disp == CREATE_NEW) + created = true; + + if (hfile == INVALID_HANDLE_VALUE) + ereport(elevel, + (errmsg("could not open injection point state file \"%s\": error code %lu", + INJ_STATE_FILE, GetLastError()))); + + /* CreateFileMapping extends the backing file to the mapping size. */ + hmap = CreateFileMapping(hfile, NULL, PAGE_READWRITE, 0, (DWORD) size, NULL); + if (hmap == NULL) + { + CloseHandle(hfile); + ereport(elevel, + (errmsg("could not create mapping for injection point state file \"%s\": error code %lu", + INJ_STATE_FILE, GetLastError()))); + } + + inj_state = MapViewOfFile(hmap, FILE_MAP_ALL_ACCESS, 0, 0, size); + CloseHandle(hmap); + CloseHandle(hfile); + + if (inj_state == NULL) + ereport(elevel, + (errmsg("could not map injection point state file \"%s\": error code %lu", + INJ_STATE_FILE, GetLastError()))); + } +#endif + + if (created) + { + injection_point_init_state(inj_state); + on_proc_exit(injection_state_file_cleanup, 0); + } } +/* + * Shared memory callbacks. We do not request any space in the main segment; + * the file mapping is the source of truth. init_fn runs once in the + * postmaster (children inherit the mapping through fork), while attach_fn + * re-maps the file in children that do not inherit it (EXEC_BACKEND/Windows). + */ static void injection_shmem_init(void *arg) { - /* - * First time through, so initialize. This is shared with the dynamic - * initialization using a DSM. - */ - injection_point_init_state(inj_state, NULL); + injection_map_state(INJ_MAP_CREATE, FATAL); +} + +static void +injection_shmem_attach(void *arg) +{ + injection_map_state(INJ_MAP_ATTACH, FATAL); } /* - * Initialize shared memory area for this module through DSM. + * Ensure inj_state is available in the current process. + * + * Backends preloading the module inherit (fork) or re-map (EXEC_BACKEND) the + * state set up at postmaster startup. When the module is not preloaded, the + * first process to reach here creates the file and the rest attach to it. */ static void injection_init_shmem(void) { - bool found; - if (inj_state != NULL) return; - inj_state = GetNamedDSMSegment("injection_points", - sizeof(InjectionPointSharedState), - injection_point_init_state, - &found, NULL); + injection_map_state(INJ_MAP_ATTACH_OR_CREATE, ERROR); } /* diff --git a/src/test/modules/injection_points/injection_points.h b/src/test/modules/injection_points/injection_points.h index caabc4ffb32..560bede9d45 100644 --- a/src/test/modules/injection_points/injection_points.h +++ b/src/test/modules/injection_points/injection_points.h @@ -15,6 +15,45 @@ #ifndef INJECTION_POINTS_H #define INJECTION_POINTS_H +#include <stdint.h> + +/* Maximum number of waits usable in injection points at once */ +#define INJ_MAX_WAIT 8 +#define INJ_NAME_MAXLEN 64 + +/* + * Name of the file under the data directory holding this module's wait/wakeup + * state. The state is mapped from this file (see injection_points.c) so that + * external programs without a backend connection can observe and release + * waiting processes (see injection_points_state.c). + * + * This is distinct from the core registry file (injection_points.shm, see + * src/backend/utils/misc/injection_point.c) that records which points are + * attached: the registry says *which* points exist, this file coordinates the + * wait/wakeup of points whose action is "wait". + */ +#define INJ_STATE_FILE "injection_points_wait.shm" + +/* + * Publicly-mappable portion of the injection point shared state. + * + * This describes the layout that external tools rely on when mapping + * INJ_STATE_FILE. It must stay at the very front of the backend-only + * InjectionPointSharedState (see injection_points.c, which static-asserts + * the layout), and must only use types that are also available to frontend + * code: no slock_t, no pg_atomic_uint32. + * + * "name" holds the injection point name registered by a waiting process in + * its slot, or an empty string if the slot is free. "wait_counts" is bumped + * to release the waiter occupying the matching slot; a 32-bit aligned counter + * is binary-compatible with the backend's pg_atomic_uint32. + */ +typedef struct InjectionPointPublicState +{ + char name[INJ_MAX_WAIT][INJ_NAME_MAXLEN]; + uint32_t wait_counts[INJ_MAX_WAIT]; +} InjectionPointPublicState; + typedef enum InjectionPointConditionType { INJ_CONDITION_ALWAYS = 0, /* always run */ diff --git a/src/test/modules/injection_points/injection_points_state.c b/src/test/modules/injection_points/injection_points_state.c new file mode 100644 index 00000000000..9c1434936bf --- /dev/null +++ b/src/test/modules/injection_points/injection_points_state.c @@ -0,0 +1,460 @@ +/*-------------------------------------------------------------------------- + * + * injection_points_state.c + * Standalone client for the injection point shared state. + * + * This small program maps the injection point state files from a data + * directory and lets a test harness drive injection points without going + * through a backend connection or any SQL. It can: + * + * - attach/detach a "wait" injection point by writing the core registry file + * (injection_points.shm, see src/backend/utils/misc/injection_point.c), + * - detect that a process has reached a wait point and release it by + * writing this module's wait file (injection_points_wait.shm, see + * injection_points.c). + * + * That is useful when the cooperating process has no PGPROC or no wait-event + * visibility (for example the postmaster, or early startup before the SQL + * machinery is up), where SQL-driven attach/wakeup is not an option and a + * fixed sleep would be unreliable. + * + * Both files are mapped exactly like the backend does -- POSIX mmap() or, on + * Windows, a file-backed CreateFileMapping() -- because plain file reads are + * not guaranteed to be coherent with a mapped view on Windows. + * + * NB: this is prototype/test tooling. attach/detach update the registry + * without holding the backend's InjectionPointLock, so they assume no + * concurrent SQL attach/detach of the same array. That holds for the + * intended use (attaching points out of band, before or alongside controlled + * test sessions). + * + * TODO: if this outgrows test tooling, the registry should get a real + * publication protocol that is safe against concurrent writers - an + * out-of-process equivalent of InjectionPointLock, or a CAS-based claim on + * the generation counter - instead of this single-writer assumption. + * + * Usage: + * injection_points_state DATADIR attach NAME + * injection_points_state DATADIR detach NAME + * injection_points_state DATADIR wait NAME [TIMEOUT_SEC] + * injection_points_state DATADIR wakeup NAME [TIMEOUT_SEC] + * + * Exit status: 0 success, 1 usage/IO error, 2 timeout. + * + * Portions Copyright (c) 1996-2026, PostgreSQL Global Development Group + * Portions Copyright (c) 1994, Regents of the University of California + * + * IDENTIFICATION + * src/test/modules/injection_points/injection_points_state.c + * + * ------------------------------------------------------------------------- + */ + +#include <stdalign.h> +#include <stddef.h> +#include <stdint.h> +#include <stdio.h> +#include <stdlib.h> +#include <string.h> + +#ifndef WIN32 +#include <errno.h> +#include <fcntl.h> +#include <sys/mman.h> +#include <sys/stat.h> +#include <time.h> +#include <unistd.h> +#else +#include <windows.h> +#endif + +#include "injection_points.h" + +#define INJ_DEFAULT_TIMEOUT_SEC 180 +#define INJ_POLL_INTERVAL_MS 10 + +/* + * Mirror of the core active injection points array (InjectionPointsCtl / + * InjectionPointEntry in src/backend/utils/misc/injection_point.c). + * + * The backend stores that array in INJ_POINTS_FILE using pg_atomic_uint{32,64} + * for "max_inuse" and "generation". Those atomic types are layout-compatible + * with the plain integers used here (the backend static-asserts the widths), + * so we can read and write the same bytes to attach and detach points. The + * field sizes below must match the backend's INJ_*_MAXLEN / + * MAX_INJECTION_POINTS. + * + * "generation" must be alignas(8): the backend's pg_atomic_uint64 forces + * 8-byte alignment, so the entries array starts 8 bytes into the control + * struct. On an ILP32 platform a plain uint64_t is only 4-byte aligned, which + * would push entries to offset 4 and make us read and write the wrong slots. + * The _Static_assert below pins the offset so any future drift fails the build + * instead of hanging a test. + */ +#define INJ_POINTS_FILE "injection_points.shm" +#define INJ_REG_MAXPOINTS 128 +#define INJ_LIB_MAXLEN 128 +#define INJ_FUNC_MAXLEN 128 +#define INJ_PRIVATE_MAXLEN 1024 + +typedef struct InjectionRegEntry +{ + alignas(8) uint64_t generation; /* even: free, odd: in use */ + char name[INJ_NAME_MAXLEN]; + char library[INJ_LIB_MAXLEN]; + char function[INJ_FUNC_MAXLEN]; + char private_data[INJ_PRIVATE_MAXLEN]; +} InjectionRegEntry; + +typedef struct InjectionRegCtl +{ + uint32_t max_inuse; + InjectionRegEntry entries[INJ_REG_MAXPOINTS]; +} InjectionRegCtl; + +_Static_assert(offsetof(InjectionRegCtl, entries) == 8, + "registry entries must start at offset 8 to match the backend"); + +static const char *progname = "injection_points_state"; + +static void +usage(void) +{ + fprintf(stderr, + "usage: %s DATADIR {attach|detach|wait|wakeup} NAME [TIMEOUT_SEC]\n", + progname); +} + +/* Sleep for the given number of milliseconds. */ +static void +sleep_ms(int ms) +{ +#ifndef WIN32 + struct timespec ts; + + ts.tv_sec = ms / 1000; + ts.tv_nsec = (long) (ms % 1000) * 1000000L; + nanosleep(&ts, NULL); +#else + Sleep(ms); +#endif +} + +/* Atomically bump a 32-bit counter shared with the backend. */ +static void +atomic_inc_u32(volatile uint32_t *counter) +{ +#if defined(_MSC_VER) + _InterlockedIncrement((volatile long *) counter); +#else + __atomic_add_fetch(counter, 1, __ATOMIC_SEQ_CST); +#endif +} + +/* Loads/stores matching the backend's generation protocol. */ +static uint32_t +load_u32(volatile uint32_t *p) +{ +#if defined(_MSC_VER) + return (uint32_t) _InterlockedOr((volatile long *) p, 0); +#else + return __atomic_load_n(p, __ATOMIC_ACQUIRE); +#endif +} + +static void +store_u32(volatile uint32_t *p, uint32_t v) +{ +#if defined(_MSC_VER) + _InterlockedExchange((volatile long *) p, (long) v); +#else + __atomic_store_n(p, v, __ATOMIC_RELEASE); +#endif +} + +static uint64_t +load_u64(volatile uint64_t *p) +{ +#if defined(_MSC_VER) + return (uint64_t) _InterlockedOr64((volatile __int64 *) p, 0); +#else + return __atomic_load_n(p, __ATOMIC_ACQUIRE); +#endif +} + +/* Publish "generation" after the other fields are in place (release store). */ +static void +store_u64_release(volatile uint64_t *p, uint64_t v) +{ +#if defined(_MSC_VER) + _InterlockedExchange64((volatile __int64 *) p, (__int64) v); +#else + __atomic_store_n(p, v, __ATOMIC_RELEASE); +#endif +} + +/* + * Map a file of the given size read-write, the same way the backend does. + * Returns the mapped base, or NULL on failure (with a message on stderr). + */ +static void * +map_file(const char *path, size_t size) +{ +#ifndef WIN32 + int fd; + void *base; + + fd = open(path, O_RDWR, 0); + if (fd < 0) + { + fprintf(stderr, "%s: could not open \"%s\": %s\n", + progname, path, strerror(errno)); + return NULL; + } + + base = mmap(NULL, size, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0); + close(fd); + + if (base == MAP_FAILED) + { + fprintf(stderr, "%s: could not map \"%s\": %s\n", + progname, path, strerror(errno)); + return NULL; + } + return base; +#else + HANDLE hfile; + HANDLE hmap; + void *base; + + hfile = CreateFile(path, GENERIC_READ | GENERIC_WRITE, + FILE_SHARE_READ | FILE_SHARE_WRITE, + NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL); + if (hfile == INVALID_HANDLE_VALUE) + { + fprintf(stderr, "%s: could not open \"%s\": error code %lu\n", + progname, path, GetLastError()); + return NULL; + } + + hmap = CreateFileMapping(hfile, NULL, PAGE_READWRITE, 0, + (DWORD) size, NULL); + if (hmap == NULL) + { + fprintf(stderr, "%s: could not create mapping for \"%s\": error code %lu\n", + progname, path, GetLastError()); + CloseHandle(hfile); + return NULL; + } + + base = MapViewOfFile(hmap, FILE_MAP_ALL_ACCESS, 0, 0, size); + CloseHandle(hmap); + CloseHandle(hfile); + + if (base == NULL) + { + fprintf(stderr, "%s: could not map \"%s\": error code %lu\n", + progname, path, GetLastError()); + return NULL; + } + return base; +#endif +} + +/* + * Attach a "wait" injection point by adding it to the registry, mimicking + * InjectionPointAttach(): find a free slot, fill the entry, then publish it by + * flipping the generation to odd with a release store. The point fires the + * module's injection_wait() callback (private_data left zeroed, i.e. an + * unconditional INJ_CONDITION_ALWAYS condition). + */ +static int +do_attach(InjectionRegCtl *ctl, const char *name) +{ + uint32_t max_inuse = load_u32(&ctl->max_inuse); + int free_idx = -1; + InjectionRegEntry *e; + uint64_t generation; + + for (uint32_t i = 0; i < max_inuse; i++) + { + uint64_t g = load_u64(&ctl->entries[i].generation); + + if (g % 2 == 0) + { + if (free_idx < 0) + free_idx = (int) i; + } + else if (strncmp(ctl->entries[i].name, name, INJ_NAME_MAXLEN) == 0) + { + fprintf(stderr, "%s: injection point \"%s\" already attached\n", + progname, name); + return 1; + } + } + + if (free_idx < 0) + { + if (max_inuse >= INJ_REG_MAXPOINTS) + { + fprintf(stderr, "%s: too many injection points\n", progname); + return 1; + } + free_idx = (int) max_inuse; + } + + e = &ctl->entries[free_idx]; + generation = load_u64(&e->generation); /* even (free) */ + + memset(e->name, 0, INJ_NAME_MAXLEN); + strncpy(e->name, name, INJ_NAME_MAXLEN - 1); + memset(e->library, 0, INJ_LIB_MAXLEN); + strncpy(e->library, "injection_points", INJ_LIB_MAXLEN - 1); + memset(e->function, 0, INJ_FUNC_MAXLEN); + strncpy(e->function, "injection_wait", INJ_FUNC_MAXLEN - 1); + memset(e->private_data, 0, INJ_PRIVATE_MAXLEN); + + store_u64_release(&e->generation, generation + 1); /* publish (odd) */ + + if ((uint32_t) (free_idx + 1) > max_inuse) + store_u32(&ctl->max_inuse, (uint32_t) (free_idx + 1)); + + return 0; +} + +/* Detach an injection point by flipping its generation back to even. */ +static int +do_detach(InjectionRegCtl *ctl, const char *name) +{ + uint32_t max_inuse = load_u32(&ctl->max_inuse); + + for (uint32_t i = 0; i < max_inuse; i++) + { + uint64_t g = load_u64(&ctl->entries[i].generation); + + if (g % 2 == 0) + continue; + if (strncmp(ctl->entries[i].name, name, INJ_NAME_MAXLEN) == 0) + { + store_u64_release(&ctl->entries[i].generation, g + 1); + return 0; + } + } + + fprintf(stderr, "%s: injection point \"%s\" not attached\n", + progname, name); + return 1; +} + +/* + * Return the wait slot currently registered for "name", or -1 if none. + * + * The backend writes the name under a spinlock before it starts waiting, so a + * stable match means the wait point has been reached. A torn read simply + * fails to match and the caller retries. + */ +static int +find_wait_slot(InjectionPointPublicState *state, const char *name) +{ + for (int i = 0; i < INJ_MAX_WAIT; i++) + { + if (strncmp(state->name[i], name, INJ_NAME_MAXLEN) == 0) + return i; + } + return -1; +} + +/* + * Poll the wait file until "name" appears (a process has reached the point), + * up to timeout_sec. Returns the slot, or -1 on timeout. + */ +static int +wait_for_slot(InjectionPointPublicState *state, const char *name, + int timeout_sec) +{ + int max_polls = (timeout_sec * 1000) / INJ_POLL_INTERVAL_MS; + + for (int polls = 0;; polls++) + { + int slot = find_wait_slot(state, name); + + if (slot >= 0) + return slot; + if (polls >= max_polls) + return -1; + sleep_ms(INJ_POLL_INTERVAL_MS); + } +} + +int +main(int argc, char **argv) +{ + const char *datadir; + const char *mode; + const char *name; + int timeout_sec = INJ_DEFAULT_TIMEOUT_SEC; + char path[1024]; + + if (argc < 4 || argc > 5) + { + usage(); + return 1; + } + + datadir = argv[1]; + mode = argv[2]; + name = argv[3]; + if (argc == 5) + timeout_sec = atoi(argv[4]); + + if (strlen(name) >= INJ_NAME_MAXLEN) + { + fprintf(stderr, "%s: injection point name too long\n", progname); + return 1; + } + + /* attach/detach operate on the core registry file. */ + if (strcmp(mode, "attach") == 0 || strcmp(mode, "detach") == 0) + { + InjectionRegCtl *ctl; + + snprintf(path, sizeof(path), "%s/%s", datadir, INJ_POINTS_FILE); + ctl = (InjectionRegCtl *) map_file(path, sizeof(InjectionRegCtl)); + if (ctl == NULL) + return 1; + + if (strcmp(mode, "attach") == 0) + return do_attach(ctl, name); + else + return do_detach(ctl, name); + } + + /* wait/wakeup operate on this module's wait file. */ + if (strcmp(mode, "wait") == 0 || strcmp(mode, "wakeup") == 0) + { + InjectionPointPublicState *state; + int slot; + + snprintf(path, sizeof(path), "%s/%s", datadir, INJ_STATE_FILE); + state = (InjectionPointPublicState *) map_file(path, + sizeof(InjectionPointPublicState)); + if (state == NULL) + return 1; + + slot = wait_for_slot(state, name, timeout_sec); + if (slot < 0) + { + fprintf(stderr, "%s: timed out waiting for injection point \"%s\"\n", + progname, name); + return 2; + } + + if (strcmp(mode, "wakeup") == 0) + atomic_inc_u32(&state->wait_counts[slot]); + + return 0; + } + + usage(); + return 1; +} diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build index 59dba1cb023..605dabb3ee8 100644 --- a/src/test/modules/injection_points/meson.build +++ b/src/test/modules/injection_points/meson.build @@ -21,6 +21,17 @@ injection_points = shared_module('injection_points', ) test_install_libs += injection_points +# Standalone client used by the TAP tests to wait on and wake up injection +# points without a backend connection. It only needs the module's own header. +injection_points_state = executable('injection_points_state', + files('injection_points_state.c'), + include_directories: include_directories('.'), + kwargs: default_bin_args + { + 'install': false, + }, +) +testprep_targets += injection_points_state + test_install_data += files( 'injection_points.control', 'injection_points--1.0.sql', @@ -30,6 +41,16 @@ tests += { 'name': 'injection_points', 'sd': meson.current_source_dir(), 'bd': meson.current_build_dir(), + 'tap': { + 'env': { + 'enable_injection_points': get_option('injection_points') ? 'yes' : 'no', + 'INJECTION_POINTS_STATE': injection_points_state.full_path(), + }, + 'tests': [ + 't/001_wait_without_sql.pl', + ], + 'deps': [injection_points_state], + }, 'regress': { 'sql': [ 'injection_points', diff --git a/src/test/modules/injection_points/t/001_wait_without_sql.pl b/src/test/modules/injection_points/t/001_wait_without_sql.pl new file mode 100644 index 00000000000..9a1cce1c1cf --- /dev/null +++ b/src/test/modules/injection_points/t/001_wait_without_sql.pl @@ -0,0 +1,128 @@ +# Copyright (c) 2026, PostgreSQL Global Development Group + +# Drive an injection point entirely from outside the server, without issuing +# any SQL to attach or coordinate it. Two files in the data directory back the +# shared state: +# +# - injection_points.shm: the core registry of attached points (see +# src/backend/utils/misc/injection_point.c). Writing it attaches a point. +# - injection_points_wait.shm: this module's wait/wakeup coordination (see +# injection_points.c). +# +# The standalone injection_points_state client maps these files the same way +# the backend does and is able to attach a "wait" point, detect that a process +# reached it, release it, and detach it -- all without a backend connection. +# This is the synchronization primitive needed when the cooperating process +# has no PGPROC or no wait-event visibility (postmaster, early startup, ...), +# where SQL-driven attach/wakeup is not available and a fixed sleep would be +# unreliable. SQL is used here only to *trigger* the point (the code path that +# would normally contain the INJECTION_POINT() macro) and to observe state. + +use strict; +use warnings FATAL => 'all'; + +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; +use Test::More; + +if ($ENV{enable_injection_points} ne 'yes') +{ + plan skip_all => 'Injection points not supported by this build'; +} + +my $client = $ENV{INJECTION_POINTS_STATE}; +if (!defined $client || $client eq '') +{ + plan skip_all => 'injection_points_state client not available'; +} + +# Preload the module so the wait file is created at startup and the library is +# available in every backend. +my $node = PostgreSQL::Test::Cluster->new('main'); +$node->init; +$node->append_conf('postgresql.conf', + "shared_preload_libraries = 'injection_points'"); +$node->start; +$node->safe_psql('postgres', 'CREATE EXTENSION injection_points;'); + +my $datadir = $node->data_dir; + +# Both backing files must exist as soon as the server is up. +ok(-f "$datadir/injection_points.shm", + 'core registry file created at startup'); +ok(-f "$datadir/injection_points_wait.shm", + 'wait state file created at startup'); + +# Attach a "wait" injection point by writing the registry file directly, with +# no SQL involved. +$node->command_ok( + [ $client, $datadir, 'attach', 'external-wait' ], + 'external client attached a wait point without SQL'); + +# The backend must see the externally-attached point in its registry. +my $listed = $node->safe_psql('postgres', + "SELECT point_name || ',' || library || ',' || function" + . " FROM injection_points_list() WHERE point_name = 'external-wait';"); +is( $listed, + 'external-wait,injection_points,injection_wait', + 'backend sees the externally-attached injection point'); + +# Trigger the point from a background session, which blocks in injection_wait(). +my $session = $node->background_psql('postgres', on_error_stop => 0); +$session->query_until( + qr/start/, qq[ + \\echo start + SELECT injection_points_run('external-wait'); +]); + +# Detect that the wait point was reached. The client polls the mapped wait +# file instead of guessing with a sleep, so it behaves the same on fast and +# slow machines. +$node->command_ok( + [ $client, $datadir, 'wait', 'external-wait' ], + 'external client detected the wait point without SQL'); + +# Detach the point *before* waking the waiter. In a code path that runs +# INJECTION_POINT() in a loop, waking first would let the woken process loop +# back and immediately re-enter the wait at the same still-attached point, so +# the robust order is detach-then-wake. Here the point is run once so the +# order is not strictly required, but the test models the correct pattern. +# Detach only flips the registry generation; the waiter stays blocked on the +# separate wait file until we bump its counter below. +$node->command_ok( + [ $client, $datadir, 'detach', 'external-wait' ], + 'external client detached the point without SQL'); + +# The backend must no longer see it in the registry, even though a process is +# still blocked at the point. +my $still = $node->safe_psql('postgres', + "SELECT count(*) FROM injection_points_list()" + . " WHERE point_name = 'external-wait';"); +is($still, '0', 'backend no longer sees the detached injection point'); + +# Release the waiter by bumping its counter through the mapped wait file, again +# without any SQL or backend connection. +$node->command_ok( + [ $client, $datadir, 'wakeup', 'external-wait' ], + 'external client woke the waiter without SQL'); + +# The blocked SELECT must now finish. +$session->query_safe('SELECT 1;'); +$session->quit; + +# A wait for the now-cleared point must time out rather than block forever, +# proving the tool reflects live state. +$node->command_fails_like( + [ $client, $datadir, 'wait', 'external-wait', '1' ], + qr/timed out/, + 'external client times out when no process waits'); + +$node->stop; + +# Both backing files are removed together with the cluster. +ok(!-f "$datadir/injection_points.shm", + 'core registry file removed at shutdown'); +ok(!-f "$datadir/injection_points_wait.shm", + 'wait state file removed at shutdown'); + +done_testing(); -- 2.50.1 (Apple Git-155) [application/octet-stream] v2026-06-12-0002-injection_points-back-the-active-points-.patch (10.1K, ../../[email protected]/4-v2026-06-12-0002-injection_points-back-the-active-points-.patch) download | inline diff: From 325d362eba5643b5b50c22fd01e013f84567cee2 Mon Sep 17 00:00:00 2001 From: Andrey Borodin <[email protected]> Date: Fri, 12 Jun 2026 10:38:21 +0500 Subject: [PATCH v2026-06-12 2/3] injection_points: back the active points array with a file Map ActiveInjectionPoints from a file in the data directory (injection_points.shm, overridable with PG_INJECTION_POINTS_FILE) instead of the main shared memory segment, so out-of-process tools can read and attach injection points with no backend connection. The lock-free generation protocol used to read the array is unchanged. --- src/backend/utils/misc/injection_point.c | 253 +++++++++++++++++++++-- 1 file changed, 239 insertions(+), 14 deletions(-) diff --git a/src/backend/utils/misc/injection_point.c b/src/backend/utils/misc/injection_point.c index 272ef5e578a..7c7260e336b 100644 --- a/src/backend/utils/misc/injection_point.c +++ b/src/backend/utils/misc/injection_point.c @@ -21,11 +21,17 @@ #ifdef USE_INJECTION_POINTS +#include <fcntl.h> #include <sys/stat.h> +#include <unistd.h> +#ifndef WIN32 +#include <sys/mman.h> +#endif #include "fmgr.h" #include "miscadmin.h" #include "storage/fd.h" +#include "storage/ipc.h" #include "storage/lwlock.h" #include "storage/shmem.h" #include "storage/subsystems.h" @@ -72,6 +78,17 @@ typedef struct InjectionPointEntry char private_data[INJ_PRIVATE_MAXLEN]; } InjectionPointEntry; +/* + * The active points array is mapped from a file (see below) that out-of-process + * tools can read and write. Those tools cannot use the pg_atomic_* API, so + * they mirror this layout with plain integers; make sure the atomic types stay + * layout-compatible with their underlying width. + */ +StaticAssertDecl(sizeof(pg_atomic_uint32) == sizeof(uint32), + "pg_atomic_uint32 must be layout-compatible with uint32"); +StaticAssertDecl(sizeof(pg_atomic_uint64) == sizeof(uint64), + "pg_atomic_uint64 must be layout-compatible with uint64"); + #define MAX_INJECTION_POINTS 128 /* @@ -87,8 +104,38 @@ typedef struct InjectionPointsCtl InjectionPointEntry entries[MAX_INJECTION_POINTS]; } InjectionPointsCtl; +/* + * The 8-byte-aligned generation counter pushes the entries array to offset 8, + * past max_inuse and its padding. Out-of-process tools mirror this, so pin it + * here too: this offset is part of the on-file contract. + */ +StaticAssertDecl(offsetof(InjectionPointsCtl, entries) == 8, + "InjectionPointsCtl.entries must start at offset 8"); + NON_EXEC_STATIC InjectionPointsCtl *ActiveInjectionPoints; +/* + * Name of the file backing the active injection points array. + * + * Unlike the rest of shared memory, this array lives in an ordinary file so + * that out-of-process tools (with no backend connection, no SQL, and possibly + * running before or instead of the postmaster) can map the same bytes, attach + * injection points and coordinate with the processes that hit them. The path + * defaults to this name relative to the data directory, but can be overridden + * with the PG_INJECTION_POINTS_FILE environment variable so that points can be + * attached even before initdb has created a data directory (e.g. for + * single-user mode bootstrap). + */ +#define INJ_POINTS_FILE "injection_points.shm" +#define INJ_POINTS_FILE_ENV "PG_INJECTION_POINTS_FILE" + +/* How injection_map_points() should open the backing file. */ +typedef enum InjectionMapMode +{ + INJ_MAP_ATTACH, /* map an already-existing file */ + INJ_MAP_ATTACH_OR_CREATE, /* attach if present, else create */ +} InjectionMapMode; + /* * Backend local cache of injection callbacks already loaded, stored in * TopMemoryContext. @@ -110,8 +157,9 @@ typedef struct InjectionPointCacheEntry static HTAB *InjectionPointCache = NULL; -static void InjectionPointShmemRequest(void *arg); -static void InjectionPointShmemInit(void *arg); +static void injection_shmem_init(void *arg); +static void injection_shmem_attach(void *arg); +static void injection_map_points(InjectionMapMode mode, int elevel); /* * injection_point_cache_add @@ -229,29 +277,206 @@ injection_point_cache_get(const char *name) return NULL; } +/* + * The active injection points array is backed by a file rather than the main + * shared memory segment, so we do not reserve any space there. init_fn maps + * (and, if needed, creates) the file once in the postmaster; children inherit + * the mapping through fork(), while attach_fn re-maps it in children that do + * not (EXEC_BACKEND/Windows). + */ const ShmemCallbacks InjectionPointShmemCallbacks = { - .request_fn = InjectionPointShmemRequest, - .init_fn = InjectionPointShmemInit, + .init_fn = injection_shmem_init, + .attach_fn = injection_shmem_attach, }; /* - * Reserve space for the dynamic shared hash table + * Resolve the path of the backing file. The result is cached in a static + * buffer so that the cleanup callback can unlink the same path that was + * mapped, regardless of later CWD or environment changes. */ -static void -InjectionPointShmemRequest(void *arg) +static const char * +injection_points_file_path(void) { - ShmemRequestStruct(.name = "InjectionPoint hash", - .size = sizeof(InjectionPointsCtl), - .ptr = (void **) &ActiveInjectionPoints, - ); + static char path[MAXPGPATH]; + const char *env; + + if (path[0] != '\0') + return path; + + env = getenv(INJ_POINTS_FILE_ENV); + if (env != NULL && env[0] != '\0') + strlcpy(path, env, sizeof(path)); + else + strlcpy(path, INJ_POINTS_FILE, sizeof(path)); + + return path; } +/* + * Initialize a freshly-created array. + */ static void -InjectionPointShmemInit(void *arg) +injection_points_init_ctl(InjectionPointsCtl *ctl) { - pg_atomic_init_u32(&ActiveInjectionPoints->max_inuse, 0); + pg_atomic_init_u32(&ctl->max_inuse, 0); for (int i = 0; i < MAX_INJECTION_POINTS; i++) - pg_atomic_init_u64(&ActiveInjectionPoints->entries[i].generation, 0); + pg_atomic_init_u64(&ctl->entries[i].generation, 0); +} + +/* + * proc_exit callback that drops the mapping and, in the process that owns the + * cluster lifecycle (the postmaster, or a standalone backend), unlinks the + * backing file so it does not survive the cluster. Forked children inherit + * this callback but must not remove the file. + */ +static void +injection_points_file_cleanup(int code, Datum arg) +{ + if (ActiveInjectionPoints != NULL) + { +#ifndef WIN32 + munmap(ActiveInjectionPoints, sizeof(InjectionPointsCtl)); +#else + UnmapViewOfFile(ActiveInjectionPoints); +#endif + ActiveInjectionPoints = NULL; + } + + if (!IsUnderPostmaster) + (void) unlink(injection_points_file_path()); +} + +/* + * Map the backing file into this process, creating and/or initializing it as + * dictated by "mode", and set ActiveInjectionPoints. + * + * Accessors must always use the mapping, never plain file reads, because file + * I/O is not guaranteed to be coherent with a mapped view on Windows. + */ +static void +injection_map_points(InjectionMapMode mode, int elevel) +{ + const char *path = injection_points_file_path(); + Size size = sizeof(InjectionPointsCtl); + bool created = false; + + if (ActiveInjectionPoints != NULL) + return; + +#ifndef WIN32 + { + int fd; + int oflags = O_RDWR; + + if (mode != INJ_MAP_ATTACH) + oflags |= O_CREAT | O_EXCL; + + fd = OpenTransientFile(path, oflags); + if (fd < 0 && mode == INJ_MAP_ATTACH_OR_CREATE && errno == EEXIST) + { + /* Lost the race to create it; just attach. */ + oflags = O_RDWR; + fd = OpenTransientFile(path, oflags); + } + else if (fd >= 0 && (oflags & O_CREAT)) + created = true; + + if (fd < 0) + ereport(elevel, + (errcode_for_file_access(), + errmsg("could not open injection point file \"%s\": %m", + path))); + + if (created && ftruncate(fd, size) != 0) + { + CloseTransientFile(fd); + (void) unlink(path); + ereport(elevel, + (errcode_for_file_access(), + errmsg("could not size injection point file \"%s\": %m", + path))); + } + + ActiveInjectionPoints = mmap(NULL, size, PROT_READ | PROT_WRITE, + MAP_SHARED, fd, 0); + CloseTransientFile(fd); + + if (ActiveInjectionPoints == MAP_FAILED) + { + ActiveInjectionPoints = NULL; + ereport(elevel, + (errcode_for_file_access(), + errmsg("could not map injection point file \"%s\": %m", + path))); + } + } +#else + { + HANDLE hfile; + HANDLE hmap; + DWORD disp = (mode == INJ_MAP_ATTACH) ? OPEN_EXISTING : CREATE_NEW; + + hfile = CreateFile(path, + GENERIC_READ | GENERIC_WRITE, + FILE_SHARE_READ | FILE_SHARE_WRITE, + NULL, disp, FILE_ATTRIBUTE_NORMAL, NULL); + if (hfile == INVALID_HANDLE_VALUE && + mode == INJ_MAP_ATTACH_OR_CREATE && + GetLastError() == ERROR_FILE_EXISTS) + { + disp = OPEN_EXISTING; + hfile = CreateFile(path, + GENERIC_READ | GENERIC_WRITE, + FILE_SHARE_READ | FILE_SHARE_WRITE, + NULL, disp, FILE_ATTRIBUTE_NORMAL, NULL); + } + else if (hfile != INVALID_HANDLE_VALUE && disp == CREATE_NEW) + created = true; + + if (hfile == INVALID_HANDLE_VALUE) + ereport(elevel, + (errmsg("could not open injection point file \"%s\": error code %lu", + path, GetLastError()))); + + /* CreateFileMapping extends the backing file to the mapping size. */ + hmap = CreateFileMapping(hfile, NULL, PAGE_READWRITE, 0, + (DWORD) size, NULL); + if (hmap == NULL) + { + CloseHandle(hfile); + ereport(elevel, + (errmsg("could not create mapping for injection point file \"%s\": error code %lu", + path, GetLastError()))); + } + + ActiveInjectionPoints = MapViewOfFile(hmap, FILE_MAP_ALL_ACCESS, 0, 0, size); + CloseHandle(hmap); + CloseHandle(hfile); + + if (ActiveInjectionPoints == NULL) + ereport(elevel, + (errmsg("could not map injection point file \"%s\": error code %lu", + path, GetLastError()))); + } +#endif + + if (created) + { + injection_points_init_ctl(ActiveInjectionPoints); + on_proc_exit(injection_points_file_cleanup, 0); + } +} + +static void +injection_shmem_init(void *arg) +{ + injection_map_points(INJ_MAP_ATTACH_OR_CREATE, FATAL); +} + +static void +injection_shmem_attach(void *arg) +{ + injection_map_points(INJ_MAP_ATTACH, FATAL); } #endif /* USE_INJECTION_POINTS */ -- 2.50.1 (Apple Git-155) ^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: injection_points: Switch wait/wakeup to use atomics rather than latches @ 2026-06-14 09:23 Andrey Borodin <[email protected]> parent: Andrey Borodin <[email protected]> 0 siblings, 1 reply; 21+ messages in thread From: Andrey Borodin @ 2026-06-14 09:23 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Robert Haas <[email protected]>; Postgres hackers <[email protected]>; Heikki Linnakangas <[email protected]> > On 12 Jun 2026, at 12:02, Andrey Borodin <[email protected]> wrote: > > I hacked up a prototype (on top of > your atomics commit) that maps the state portably on both sides v2026-06-12 passed locally but tripped on the Windows CI (EXEC_BACKEND), v2026-06-14 fixes that. The mistake was handing ActiveInjectionPoints to children as the postmaster's pointer through BackendParameters. That only works for the main shared memory segment, which is re-attached at a fixed address; the file-backed array is mapped wherever each process lands, so the inherited pointer was garbage and the first point hit in a child crashed. v2026-06-14 drops that and lets each process map the file itself. Rather than racing to map it at the right moment in child startup, the registry is mapped lazily on first use: the first time a process checks a point it attaches the file if it exists, and treats "no file" as "nothing armed". So a point is reached even when it fires before the child has attached shared memory - e.g. "backend-initialize", which 005_negotiate_encryption exercises - and a point armed out of band, even before the server is up, is not silently missed. Forked children still just inherit the postmaster's mapping. Personally I do not like lazy initialization, it's race-prone. But it guarantees the file is mapped before the first point is checked, wherever that point sits. The same revision also closes an unrelated init race: when two processes create the backing file at once, the loser could map it before the winner sized it (SIG-something on first touch). The attach path now waits for the file to reach full size before mapping. CI is green. Same two patches on top of your atomics commit. Let me know if this prototype goes radically wrong way. Thanks! Best regards, Andrey Borodin. Attachments: [application/octet-stream] v2026-06-14-0001-injection_points-Switch-wait-wakeup-to-r.patch (4.9K, ../../[email protected]/2-v2026-06-14-0001-injection_points-Switch-wait-wakeup-to-r.patch) download | inline diff: From 80cb6d9463194d887fc88a3567a82e8322b5f6c1 Mon Sep 17 00:00:00 2001 From: Michael Paquier <[email protected]> Date: Thu, 28 May 2026 11:15:33 +0900 Subject: [PATCH v2026-06-14 1/3] injection_points: Switch wait/wakeup to rely on atomics This removes the dependency based on counters and environment variables, replacing the waiting loop by a wait on an atomic counter, whose check increases over time in an exponential manner (starts at 10us, up to 100ms). --- .../injection_points/injection_points.c | 53 ++++++++++--------- 1 file changed, 27 insertions(+), 26 deletions(-) diff --git a/src/test/modules/injection_points/injection_points.c b/src/test/modules/injection_points/injection_points.c index ba282e3dcab..9b8e1aaad0b 100644 --- a/src/test/modules/injection_points/injection_points.c +++ b/src/test/modules/injection_points/injection_points.c @@ -23,11 +23,11 @@ #include "miscadmin.h" #include "nodes/pg_list.h" #include "nodes/value.h" -#include "storage/condition_variable.h" #include "storage/dsm_registry.h" #include "storage/ipc.h" #include "storage/lwlock.h" #include "storage/shmem.h" +#include "storage/spin.h" #include "utils/builtins.h" #include "utils/guc.h" #include "utils/injection_point.h" @@ -59,13 +59,10 @@ typedef struct InjectionPointSharedState slock_t lock; /* Counters advancing when injection_points_wakeup() is called */ - uint32 wait_counts[INJ_MAX_WAIT]; + pg_atomic_uint32 wait_counts[INJ_MAX_WAIT]; /* Names of injection points attached to wait counters */ char name[INJ_MAX_WAIT][INJ_NAME_MAXLEN]; - - /* Condition variable used for waits and wakeups */ - ConditionVariable wait_point; } InjectionPointSharedState; /* Pointer to shared-memory state. */ @@ -102,9 +99,9 @@ injection_point_init_state(void *ptr, void *arg) InjectionPointSharedState *state = (InjectionPointSharedState *) ptr; SpinLockInit(&state->lock); - memset(state->wait_counts, 0, sizeof(state->wait_counts)); memset(state->name, 0, sizeof(state->name)); - ConditionVariableInit(&state->wait_point); + for (int i = 0; i < INJ_MAX_WAIT; i++) + pg_atomic_init_u32(&state->wait_counts[i], 0); } static void @@ -222,7 +219,7 @@ injection_notice(const char *name, const void *private_data, void *arg) elog(NOTICE, "notice triggered for injection point %s", name); } -/* Wait on a condition variable, awaken by injection_points_wakeup() */ +/* Wait until injection_points_wakeup() is called */ void injection_wait(const char *name, const void *private_data, void *arg) { @@ -254,31 +251,37 @@ injection_wait(const char *name, const void *private_data, void *arg) { index = i; strlcpy(inj_state->name[i], name, INJ_NAME_MAXLEN); - old_wait_counts = inj_state->wait_counts[i]; + old_wait_counts = pg_atomic_read_u32(&inj_state->wait_counts[i]); break; } } SpinLockRelease(&inj_state->lock); if (index < 0) - elog(ERROR, "could not find free slot for wait of injection point %s ", + elog(ERROR, "could not find free slot for wait of injection point %s", name); - /* And sleep.. */ - ConditionVariablePrepareToSleep(&inj_state->wait_point); - for (;;) + /* + * Wait until the counter is bumped by injection_points_wakeup(). + * + * This loop starts with a short delay for responsiveness, enlarged to + * ease the CPU workload in slower environments. + */ +#define INJ_WAIT_INITIAL_US 10 /* 10us */ +#define INJ_WAIT_MAX_US 100000 /* 100ms */ + pgstat_report_wait_start(injection_wait_event); { - uint32 new_wait_counts; + int delay_us = INJ_WAIT_INITIAL_US; - SpinLockAcquire(&inj_state->lock); - new_wait_counts = inj_state->wait_counts[index]; - SpinLockRelease(&inj_state->lock); - - if (old_wait_counts != new_wait_counts) - break; - ConditionVariableSleep(&inj_state->wait_point, injection_wait_event); + while (pg_atomic_read_u32(&inj_state->wait_counts[index]) == old_wait_counts) + { + CHECK_FOR_INTERRUPTS(); + pg_usleep(delay_us); + if (delay_us < INJ_WAIT_MAX_US) + delay_us *= 2; + } } - ConditionVariableCancelSleep(); + pgstat_report_wait_end(); /* Remove this injection point from the waiters. */ SpinLockAcquire(&inj_state->lock); @@ -443,7 +446,7 @@ injection_points_wakeup(PG_FUNCTION_ARGS) if (inj_state == NULL) injection_init_shmem(); - /* First bump the wait counter for the injection point to wake up */ + /* Find the injection point then bump its wait counter */ SpinLockAcquire(&inj_state->lock); for (int i = 0; i < INJ_MAX_WAIT; i++) { @@ -458,11 +461,9 @@ injection_points_wakeup(PG_FUNCTION_ARGS) SpinLockRelease(&inj_state->lock); elog(ERROR, "could not find injection point %s to wake up", name); } - inj_state->wait_counts[index]++; SpinLockRelease(&inj_state->lock); - /* And broadcast the change to the waiters */ - ConditionVariableBroadcast(&inj_state->wait_point); + pg_atomic_fetch_add_u32(&inj_state->wait_counts[index], 1); PG_RETURN_VOID(); } -- 2.50.1 (Apple Git-155) [application/octet-stream] v2026-06-14-0003-injection_points-attach-and-coordinate-w.patch (37.3K, ../../[email protected]/3-v2026-06-14-0003-injection_points-attach-and-coordinate-w.patch) download | inline diff: From bca427fbf799a7ca2f348f82198ad706ab44c957 Mon Sep 17 00:00:00 2001 From: Andrey Borodin <[email protected]> Date: Sat, 13 Jun 2026 13:05:29 +0500 Subject: [PATCH v2026-06-14 3/3] injection_points: attach and coordinate wait points without SQL Move this module's wait/wakeup state out of a DSM into its own file (injection_points_wait.shm) and add injection_points_state, a small client that maps both files the way the backend does to attach/detach a wait point and to detect and release a waiter, all without SQL. A TAP test exercises the no-SQL flow. --- src/test/modules/injection_points/.gitignore | 3 + src/test/modules/injection_points/Makefile | 21 + .../injection_points/injection_points.c | 321 ++++++++++-- .../injection_points/injection_points.h | 39 ++ .../injection_points/injection_points_state.c | 460 ++++++++++++++++++ src/test/modules/injection_points/meson.build | 21 + .../t/001_wait_without_sql.pl | 128 +++++ 7 files changed, 957 insertions(+), 36 deletions(-) create mode 100644 src/test/modules/injection_points/injection_points_state.c create mode 100644 src/test/modules/injection_points/t/001_wait_without_sql.pl diff --git a/src/test/modules/injection_points/.gitignore b/src/test/modules/injection_points/.gitignore index 0de307e70a6..88fccf29987 100644 --- a/src/test/modules/injection_points/.gitignore +++ b/src/test/modules/injection_points/.gitignore @@ -4,3 +4,6 @@ /results/ /tmp_check/ /tmp_check_iso/ + +# Standalone state client +/injection_points_state diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile index c01d2fb095c..b7387abe84c 100644 --- a/src/test/modules/injection_points/Makefile +++ b/src/test/modules/injection_points/Makefile @@ -24,10 +24,19 @@ ISOLATION = basic \ # some isolation tests require wal_level=replica ISOLATION_OPTS = --temp-config $(top_srcdir)/src/test/modules/injection_points/extra.conf +TAP_TESTS = 1 + # The injection points are cluster-wide, so disable installcheck NO_INSTALLCHECK = 1 +# Standalone client used by the TAP tests to wait on and wake up injection +# points without a backend connection. It is built alongside the module +# (MODULE_big already owns OBJS, so it cannot go through PGXS PROGRAM) and its +# absolute path is exported for the TAP tests. +INJ_STATE_CLIENT = injection_points_state$(X) + export enable_injection_points +export INJECTION_POINTS_STATE := $(abspath $(INJ_STATE_CLIENT)) ifdef USE_PGXS PG_CONFIG = pg_config @@ -51,3 +60,15 @@ check: endif endif + +# Build the standalone state client. Its object is compiled by the implicit +# rule (which adds -I. so injection_points.h is found) and it links against no +# backend libraries. +all: $(INJ_STATE_CLIENT) + +$(INJ_STATE_CLIENT): injection_points_state.o + $(CC) $(CFLAGS) $< $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@ + +clean: clean-injection-points-state +clean-injection-points-state: + rm -f $(INJ_STATE_CLIENT) injection_points_state.o diff --git a/src/test/modules/injection_points/injection_points.c b/src/test/modules/injection_points/injection_points.c index 9b8e1aaad0b..b24a3035e2a 100644 --- a/src/test/modules/injection_points/injection_points.c +++ b/src/test/modules/injection_points/injection_points.c @@ -17,13 +17,19 @@ #include "postgres.h" +#include <fcntl.h> +#include <sys/stat.h> +#ifndef WIN32 +#include <sys/mman.h> +#endif + #include "fmgr.h" #include "funcapi.h" #include "injection_points.h" #include "miscadmin.h" #include "nodes/pg_list.h" #include "nodes/value.h" -#include "storage/dsm_registry.h" +#include "storage/fd.h" #include "storage/ipc.h" #include "storage/lwlock.h" #include "storage/shmem.h" @@ -37,10 +43,6 @@ PG_MODULE_MAGIC; -/* Maximum number of waits usable in injection points at once */ -#define INJ_MAX_WAIT 8 -#define INJ_NAME_MAXLEN 64 - /* * List of injection points stored in TopMemoryContext attached * locally to this process. @@ -50,22 +52,40 @@ static List *inj_list_local = NIL; /* * Shared state information for injection points. * - * This state data can be initialized in two ways: dynamically with a DSM - * or when loading the module. + * This is mapped from a fixed file in the data directory (INJ_STATE_FILE) + * rather than being allocated in the main shared memory segment or a DSM. + * Backing it with a file lets external programs without a backend connection + * map the same state, observe which injection points are being waited on and + * release them (see injection_points_state.c). This works in contexts where + * condition variables and latches are unavailable, e.g. the postmaster or a + * process that has not set up its PGPROC yet. + * + * The leading InjectionPointPublicState portion is the contract shared with + * those external tools, so it must stay first and keep a frontend-compatible + * layout (see injection_points.h). The fields below it are backend-only. */ typedef struct InjectionPointSharedState { - /* Protects access to other fields */ - slock_t lock; + /* Names of injection points attached to wait counters (slot in use). */ + char name[INJ_MAX_WAIT][INJ_NAME_MAXLEN]; /* Counters advancing when injection_points_wakeup() is called */ pg_atomic_uint32 wait_counts[INJ_MAX_WAIT]; - /* Names of injection points attached to wait counters */ - char name[INJ_MAX_WAIT][INJ_NAME_MAXLEN]; + /* Protects access to the name array (backend-only) */ + slock_t lock; } InjectionPointSharedState; -/* Pointer to shared-memory state. */ +/* The public prefix must match InjectionPointPublicState bit for bit. */ +StaticAssertDecl(offsetof(InjectionPointSharedState, name) == 0, + "name must be the first field of InjectionPointSharedState"); +StaticAssertDecl(offsetof(InjectionPointSharedState, wait_counts) == + offsetof(InjectionPointPublicState, wait_counts), + "wait_counts offset must match InjectionPointPublicState"); +StaticAssertDecl(sizeof(pg_atomic_uint32) == sizeof(uint32), + "pg_atomic_uint32 must be layout-compatible with uint32"); + +/* Pointer to the mapped shared state. */ static InjectionPointSharedState *inj_state = NULL; extern PGDLLEXPORT void injection_error(const char *name, @@ -81,63 +101,292 @@ 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; -static void injection_shmem_request(void *arg); +/* How injection_map_state() should open the backing file. */ +typedef enum InjectionMapMode +{ + INJ_MAP_CREATE, /* discard any stale file, create fresh */ + INJ_MAP_ATTACH, /* map an already-existing file */ + INJ_MAP_ATTACH_OR_CREATE, /* attach if present, else create */ +} InjectionMapMode; + static void injection_shmem_init(void *arg); +static void injection_shmem_attach(void *arg); static const ShmemCallbacks injection_shmem_callbacks = { - .request_fn = injection_shmem_request, + /* Create and initialize the backing file once at postmaster startup. */ .init_fn = injection_shmem_init, + /* Re-map it in each child that does not inherit the mapping (Windows). */ + .attach_fn = injection_shmem_attach, }; /* - * Routine for shared memory area initialization, used as a callback - * when initializing dynamically with a DSM or when loading the module. + * Initialize a freshly-created shared state. */ static void -injection_point_init_state(void *ptr, void *arg) +injection_point_init_state(InjectionPointSharedState *state) { - InjectionPointSharedState *state = (InjectionPointSharedState *) ptr; - SpinLockInit(&state->lock); memset(state->name, 0, sizeof(state->name)); for (int i = 0; i < INJ_MAX_WAIT; i++) pg_atomic_init_u32(&state->wait_counts[i], 0); } -static void -injection_shmem_request(void *arg) +/* + * Resolve the absolute path of the backing file. Anchored at the data + * directory rather than the current working directory: EXEC_BACKEND children + * re-map the file from attach_fn before they chdir into the data directory, so + * a relative path would resolve against the wrong directory there. This also + * matches the absolute path that out-of-process tools build from the data + * directory they are given. + */ +static const char * +injection_state_file_path(void) { - ShmemRequestStruct(.name = "injection_points", - .size = sizeof(InjectionPointSharedState), - .ptr = (void **) &inj_state, - ); + static char path[MAXPGPATH]; + + if (path[0] != '\0') + return path; + + if (DataDir != NULL && DataDir[0] != '\0') + snprintf(path, sizeof(path), "%s/%s", DataDir, INJ_STATE_FILE); + else + strlcpy(path, INJ_STATE_FILE, sizeof(path)); + + return path; } +/* + * proc_exit callback removing the backing file. Registered only by the + * process that created it (the postmaster, or a lone backend when the module + * is not preloaded), so that the file disappears together with the cluster. + */ static void -injection_shmem_init(void *arg) +injection_state_file_cleanup(int code, Datum arg) { + if (inj_state != NULL) + { +#ifndef WIN32 + munmap(inj_state, sizeof(InjectionPointSharedState)); +#else + UnmapViewOfFile(inj_state); +#endif + inj_state = NULL; + } + /* - * First time through, so initialize. This is shared with the dynamic - * initialization using a DSM. + * Only the postmaster (or a standalone backend) should unlink the file; + * forked children inherit this callback but must not remove it. */ - injection_point_init_state(inj_state, NULL); + if (!IsUnderPostmaster) + (void) unlink(injection_state_file_path()); } +#ifndef WIN32 /* - * Initialize shared memory area for this module through DSM. + * Wait for a file created by a concurrent process to reach its final size. + * + * The winner of the O_EXCL create race makes the file at length zero and only + * then ftruncate()s it to "size". A process that lost the race and is + * attaching could otherwise mmap() past end-of-file and take a SIGBUS on first + * access. The gap is just the few instructions between create and ftruncate, + * so in practice this returns on the first check. */ static void -injection_init_shmem(void) +injection_wait_for_size(int fd, Size size, const char *path, int elevel) +{ + /* Generous bound; the writer needs only microseconds. */ + for (int i = 0; i < 10000; i++) + { + struct stat st; + + if (fstat(fd, &st) != 0) + { + ereport(elevel, + (errcode_for_file_access(), + errmsg("could not stat injection point state file \"%s\": %m", + path))); + return; + } + if (st.st_size >= (off_t) size) + return; + pg_usleep(1000L); /* 1ms */ + } + + ereport(elevel, + (errmsg("injection point state file \"%s\" was never sized by its creator", + path))); +} +#endif + +/* + * Map INJ_STATE_FILE into this process, creating and/or initializing it as + * dictated by "mode", and set inj_state. + * + * The state is backed by an ordinary file so that external programs can map + * the same bytes (POSIX mmap or, on Windows, a file-backed CreateFileMapping). + * All accessors must use the mapping, never plain file reads, because file + * I/O is not guaranteed to be coherent with a mapped view on Windows. + */ +static void +injection_map_state(InjectionMapMode mode, int elevel) +{ + Size size = sizeof(InjectionPointSharedState); + const char *path = injection_state_file_path(); + bool created = false; + + if (inj_state != NULL) + return; + + if (mode == INJ_MAP_CREATE) + (void) unlink(path); /* drop any stale file from a crash */ + +#ifndef WIN32 + { + int fd; + int oflags = O_RDWR; + + if (mode != INJ_MAP_ATTACH) + oflags |= O_CREAT | O_EXCL; + + fd = OpenTransientFile(path, oflags); + if (fd < 0 && mode == INJ_MAP_ATTACH_OR_CREATE && errno == EEXIST) + { + /* Lost the race to create it; just attach. */ + oflags = O_RDWR; + fd = OpenTransientFile(path, oflags); + + /* + * The winner may not have ftruncate()d the file to full size yet; + * wait for it so the mmap() below cannot fault past end-of-file. + */ + if (fd >= 0) + injection_wait_for_size(fd, size, path, elevel); + } + else if (fd >= 0 && (oflags & O_CREAT)) + created = true; + + if (fd < 0) + ereport(elevel, + (errcode_for_file_access(), + errmsg("could not open injection point state file \"%s\": %m", + path))); + + if (created && ftruncate(fd, size) != 0) + { + CloseTransientFile(fd); + (void) unlink(path); + ereport(elevel, + (errcode_for_file_access(), + errmsg("could not size injection point state file \"%s\": %m", + path))); + } + + inj_state = mmap(NULL, size, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0); + CloseTransientFile(fd); + + if (inj_state == MAP_FAILED) + { + inj_state = NULL; + ereport(elevel, + (errcode_for_file_access(), + errmsg("could not map injection point state file \"%s\": %m", + path))); + } + } +#else + { + HANDLE hfile; + HANDLE hmap; + DWORD share = FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE; + DWORD disp = (mode == INJ_MAP_ATTACH) ? OPEN_EXISTING : CREATE_NEW; + + hfile = CreateFile(path, + GENERIC_READ | GENERIC_WRITE, + share, NULL, disp, FILE_ATTRIBUTE_NORMAL, NULL); + + /* + * If creation failed because a file from an earlier cluster lifetime + * is in the way, attach to it instead of failing. Don't insist on a + * specific error code: besides ERROR_FILE_EXISTS, a file whose + * deletion is still pending reports ERROR_ACCESS_DENIED. + * FILE_SHARE_DELETE (above) lets such a file be reopened and unlinked + * while still mapped. + */ + if (hfile == INVALID_HANDLE_VALUE && mode == INJ_MAP_ATTACH_OR_CREATE) + { + disp = OPEN_EXISTING; + hfile = CreateFile(path, + GENERIC_READ | GENERIC_WRITE, + share, NULL, disp, FILE_ATTRIBUTE_NORMAL, NULL); + } + else if (hfile != INVALID_HANDLE_VALUE && disp == CREATE_NEW) + created = true; + + if (hfile == INVALID_HANDLE_VALUE) + ereport(elevel, + (errmsg("could not open injection point state file \"%s\": error code %lu", + path, GetLastError()))); + + /* CreateFileMapping extends the backing file to the mapping size. */ + hmap = CreateFileMapping(hfile, NULL, PAGE_READWRITE, 0, (DWORD) size, NULL); + if (hmap == NULL) + { + CloseHandle(hfile); + ereport(elevel, + (errmsg("could not create mapping for injection point state file \"%s\": error code %lu", + path, GetLastError()))); + } + + inj_state = MapViewOfFile(hmap, FILE_MAP_ALL_ACCESS, 0, 0, size); + CloseHandle(hmap); + CloseHandle(hfile); + + if (inj_state == NULL) + ereport(elevel, + (errmsg("could not map injection point state file \"%s\": error code %lu", + path, GetLastError()))); + } +#endif + + if (created) + { + injection_point_init_state(inj_state); + on_proc_exit(injection_state_file_cleanup, 0); + } +} + +/* + * Shared memory callbacks. We do not request any space in the main segment; + * the file mapping is the source of truth. init_fn runs once in the + * postmaster (children inherit the mapping through fork), while attach_fn + * re-maps the file in children that do not inherit it (EXEC_BACKEND/Windows). + */ +static void +injection_shmem_init(void *arg) { - bool found; + injection_map_state(INJ_MAP_CREATE, FATAL); +} + +static void +injection_shmem_attach(void *arg) +{ + injection_map_state(INJ_MAP_ATTACH, FATAL); +} +/* + * Ensure inj_state is available in the current process. + * + * Backends preloading the module inherit (fork) or re-map (EXEC_BACKEND) the + * state set up at postmaster startup. When the module is not preloaded, the + * first process to reach here creates the file and the rest attach to it. + */ +static void +injection_init_shmem(void) +{ if (inj_state != NULL) return; - inj_state = GetNamedDSMSegment("injection_points", - sizeof(InjectionPointSharedState), - injection_point_init_state, - &found, NULL); + injection_map_state(INJ_MAP_ATTACH_OR_CREATE, ERROR); } /* diff --git a/src/test/modules/injection_points/injection_points.h b/src/test/modules/injection_points/injection_points.h index caabc4ffb32..560bede9d45 100644 --- a/src/test/modules/injection_points/injection_points.h +++ b/src/test/modules/injection_points/injection_points.h @@ -15,6 +15,45 @@ #ifndef INJECTION_POINTS_H #define INJECTION_POINTS_H +#include <stdint.h> + +/* Maximum number of waits usable in injection points at once */ +#define INJ_MAX_WAIT 8 +#define INJ_NAME_MAXLEN 64 + +/* + * Name of the file under the data directory holding this module's wait/wakeup + * state. The state is mapped from this file (see injection_points.c) so that + * external programs without a backend connection can observe and release + * waiting processes (see injection_points_state.c). + * + * This is distinct from the core registry file (injection_points.shm, see + * src/backend/utils/misc/injection_point.c) that records which points are + * attached: the registry says *which* points exist, this file coordinates the + * wait/wakeup of points whose action is "wait". + */ +#define INJ_STATE_FILE "injection_points_wait.shm" + +/* + * Publicly-mappable portion of the injection point shared state. + * + * This describes the layout that external tools rely on when mapping + * INJ_STATE_FILE. It must stay at the very front of the backend-only + * InjectionPointSharedState (see injection_points.c, which static-asserts + * the layout), and must only use types that are also available to frontend + * code: no slock_t, no pg_atomic_uint32. + * + * "name" holds the injection point name registered by a waiting process in + * its slot, or an empty string if the slot is free. "wait_counts" is bumped + * to release the waiter occupying the matching slot; a 32-bit aligned counter + * is binary-compatible with the backend's pg_atomic_uint32. + */ +typedef struct InjectionPointPublicState +{ + char name[INJ_MAX_WAIT][INJ_NAME_MAXLEN]; + uint32_t wait_counts[INJ_MAX_WAIT]; +} InjectionPointPublicState; + typedef enum InjectionPointConditionType { INJ_CONDITION_ALWAYS = 0, /* always run */ diff --git a/src/test/modules/injection_points/injection_points_state.c b/src/test/modules/injection_points/injection_points_state.c new file mode 100644 index 00000000000..9c1434936bf --- /dev/null +++ b/src/test/modules/injection_points/injection_points_state.c @@ -0,0 +1,460 @@ +/*-------------------------------------------------------------------------- + * + * injection_points_state.c + * Standalone client for the injection point shared state. + * + * This small program maps the injection point state files from a data + * directory and lets a test harness drive injection points without going + * through a backend connection or any SQL. It can: + * + * - attach/detach a "wait" injection point by writing the core registry file + * (injection_points.shm, see src/backend/utils/misc/injection_point.c), + * - detect that a process has reached a wait point and release it by + * writing this module's wait file (injection_points_wait.shm, see + * injection_points.c). + * + * That is useful when the cooperating process has no PGPROC or no wait-event + * visibility (for example the postmaster, or early startup before the SQL + * machinery is up), where SQL-driven attach/wakeup is not an option and a + * fixed sleep would be unreliable. + * + * Both files are mapped exactly like the backend does -- POSIX mmap() or, on + * Windows, a file-backed CreateFileMapping() -- because plain file reads are + * not guaranteed to be coherent with a mapped view on Windows. + * + * NB: this is prototype/test tooling. attach/detach update the registry + * without holding the backend's InjectionPointLock, so they assume no + * concurrent SQL attach/detach of the same array. That holds for the + * intended use (attaching points out of band, before or alongside controlled + * test sessions). + * + * TODO: if this outgrows test tooling, the registry should get a real + * publication protocol that is safe against concurrent writers - an + * out-of-process equivalent of InjectionPointLock, or a CAS-based claim on + * the generation counter - instead of this single-writer assumption. + * + * Usage: + * injection_points_state DATADIR attach NAME + * injection_points_state DATADIR detach NAME + * injection_points_state DATADIR wait NAME [TIMEOUT_SEC] + * injection_points_state DATADIR wakeup NAME [TIMEOUT_SEC] + * + * Exit status: 0 success, 1 usage/IO error, 2 timeout. + * + * Portions Copyright (c) 1996-2026, PostgreSQL Global Development Group + * Portions Copyright (c) 1994, Regents of the University of California + * + * IDENTIFICATION + * src/test/modules/injection_points/injection_points_state.c + * + * ------------------------------------------------------------------------- + */ + +#include <stdalign.h> +#include <stddef.h> +#include <stdint.h> +#include <stdio.h> +#include <stdlib.h> +#include <string.h> + +#ifndef WIN32 +#include <errno.h> +#include <fcntl.h> +#include <sys/mman.h> +#include <sys/stat.h> +#include <time.h> +#include <unistd.h> +#else +#include <windows.h> +#endif + +#include "injection_points.h" + +#define INJ_DEFAULT_TIMEOUT_SEC 180 +#define INJ_POLL_INTERVAL_MS 10 + +/* + * Mirror of the core active injection points array (InjectionPointsCtl / + * InjectionPointEntry in src/backend/utils/misc/injection_point.c). + * + * The backend stores that array in INJ_POINTS_FILE using pg_atomic_uint{32,64} + * for "max_inuse" and "generation". Those atomic types are layout-compatible + * with the plain integers used here (the backend static-asserts the widths), + * so we can read and write the same bytes to attach and detach points. The + * field sizes below must match the backend's INJ_*_MAXLEN / + * MAX_INJECTION_POINTS. + * + * "generation" must be alignas(8): the backend's pg_atomic_uint64 forces + * 8-byte alignment, so the entries array starts 8 bytes into the control + * struct. On an ILP32 platform a plain uint64_t is only 4-byte aligned, which + * would push entries to offset 4 and make us read and write the wrong slots. + * The _Static_assert below pins the offset so any future drift fails the build + * instead of hanging a test. + */ +#define INJ_POINTS_FILE "injection_points.shm" +#define INJ_REG_MAXPOINTS 128 +#define INJ_LIB_MAXLEN 128 +#define INJ_FUNC_MAXLEN 128 +#define INJ_PRIVATE_MAXLEN 1024 + +typedef struct InjectionRegEntry +{ + alignas(8) uint64_t generation; /* even: free, odd: in use */ + char name[INJ_NAME_MAXLEN]; + char library[INJ_LIB_MAXLEN]; + char function[INJ_FUNC_MAXLEN]; + char private_data[INJ_PRIVATE_MAXLEN]; +} InjectionRegEntry; + +typedef struct InjectionRegCtl +{ + uint32_t max_inuse; + InjectionRegEntry entries[INJ_REG_MAXPOINTS]; +} InjectionRegCtl; + +_Static_assert(offsetof(InjectionRegCtl, entries) == 8, + "registry entries must start at offset 8 to match the backend"); + +static const char *progname = "injection_points_state"; + +static void +usage(void) +{ + fprintf(stderr, + "usage: %s DATADIR {attach|detach|wait|wakeup} NAME [TIMEOUT_SEC]\n", + progname); +} + +/* Sleep for the given number of milliseconds. */ +static void +sleep_ms(int ms) +{ +#ifndef WIN32 + struct timespec ts; + + ts.tv_sec = ms / 1000; + ts.tv_nsec = (long) (ms % 1000) * 1000000L; + nanosleep(&ts, NULL); +#else + Sleep(ms); +#endif +} + +/* Atomically bump a 32-bit counter shared with the backend. */ +static void +atomic_inc_u32(volatile uint32_t *counter) +{ +#if defined(_MSC_VER) + _InterlockedIncrement((volatile long *) counter); +#else + __atomic_add_fetch(counter, 1, __ATOMIC_SEQ_CST); +#endif +} + +/* Loads/stores matching the backend's generation protocol. */ +static uint32_t +load_u32(volatile uint32_t *p) +{ +#if defined(_MSC_VER) + return (uint32_t) _InterlockedOr((volatile long *) p, 0); +#else + return __atomic_load_n(p, __ATOMIC_ACQUIRE); +#endif +} + +static void +store_u32(volatile uint32_t *p, uint32_t v) +{ +#if defined(_MSC_VER) + _InterlockedExchange((volatile long *) p, (long) v); +#else + __atomic_store_n(p, v, __ATOMIC_RELEASE); +#endif +} + +static uint64_t +load_u64(volatile uint64_t *p) +{ +#if defined(_MSC_VER) + return (uint64_t) _InterlockedOr64((volatile __int64 *) p, 0); +#else + return __atomic_load_n(p, __ATOMIC_ACQUIRE); +#endif +} + +/* Publish "generation" after the other fields are in place (release store). */ +static void +store_u64_release(volatile uint64_t *p, uint64_t v) +{ +#if defined(_MSC_VER) + _InterlockedExchange64((volatile __int64 *) p, (__int64) v); +#else + __atomic_store_n(p, v, __ATOMIC_RELEASE); +#endif +} + +/* + * Map a file of the given size read-write, the same way the backend does. + * Returns the mapped base, or NULL on failure (with a message on stderr). + */ +static void * +map_file(const char *path, size_t size) +{ +#ifndef WIN32 + int fd; + void *base; + + fd = open(path, O_RDWR, 0); + if (fd < 0) + { + fprintf(stderr, "%s: could not open \"%s\": %s\n", + progname, path, strerror(errno)); + return NULL; + } + + base = mmap(NULL, size, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0); + close(fd); + + if (base == MAP_FAILED) + { + fprintf(stderr, "%s: could not map \"%s\": %s\n", + progname, path, strerror(errno)); + return NULL; + } + return base; +#else + HANDLE hfile; + HANDLE hmap; + void *base; + + hfile = CreateFile(path, GENERIC_READ | GENERIC_WRITE, + FILE_SHARE_READ | FILE_SHARE_WRITE, + NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL); + if (hfile == INVALID_HANDLE_VALUE) + { + fprintf(stderr, "%s: could not open \"%s\": error code %lu\n", + progname, path, GetLastError()); + return NULL; + } + + hmap = CreateFileMapping(hfile, NULL, PAGE_READWRITE, 0, + (DWORD) size, NULL); + if (hmap == NULL) + { + fprintf(stderr, "%s: could not create mapping for \"%s\": error code %lu\n", + progname, path, GetLastError()); + CloseHandle(hfile); + return NULL; + } + + base = MapViewOfFile(hmap, FILE_MAP_ALL_ACCESS, 0, 0, size); + CloseHandle(hmap); + CloseHandle(hfile); + + if (base == NULL) + { + fprintf(stderr, "%s: could not map \"%s\": error code %lu\n", + progname, path, GetLastError()); + return NULL; + } + return base; +#endif +} + +/* + * Attach a "wait" injection point by adding it to the registry, mimicking + * InjectionPointAttach(): find a free slot, fill the entry, then publish it by + * flipping the generation to odd with a release store. The point fires the + * module's injection_wait() callback (private_data left zeroed, i.e. an + * unconditional INJ_CONDITION_ALWAYS condition). + */ +static int +do_attach(InjectionRegCtl *ctl, const char *name) +{ + uint32_t max_inuse = load_u32(&ctl->max_inuse); + int free_idx = -1; + InjectionRegEntry *e; + uint64_t generation; + + for (uint32_t i = 0; i < max_inuse; i++) + { + uint64_t g = load_u64(&ctl->entries[i].generation); + + if (g % 2 == 0) + { + if (free_idx < 0) + free_idx = (int) i; + } + else if (strncmp(ctl->entries[i].name, name, INJ_NAME_MAXLEN) == 0) + { + fprintf(stderr, "%s: injection point \"%s\" already attached\n", + progname, name); + return 1; + } + } + + if (free_idx < 0) + { + if (max_inuse >= INJ_REG_MAXPOINTS) + { + fprintf(stderr, "%s: too many injection points\n", progname); + return 1; + } + free_idx = (int) max_inuse; + } + + e = &ctl->entries[free_idx]; + generation = load_u64(&e->generation); /* even (free) */ + + memset(e->name, 0, INJ_NAME_MAXLEN); + strncpy(e->name, name, INJ_NAME_MAXLEN - 1); + memset(e->library, 0, INJ_LIB_MAXLEN); + strncpy(e->library, "injection_points", INJ_LIB_MAXLEN - 1); + memset(e->function, 0, INJ_FUNC_MAXLEN); + strncpy(e->function, "injection_wait", INJ_FUNC_MAXLEN - 1); + memset(e->private_data, 0, INJ_PRIVATE_MAXLEN); + + store_u64_release(&e->generation, generation + 1); /* publish (odd) */ + + if ((uint32_t) (free_idx + 1) > max_inuse) + store_u32(&ctl->max_inuse, (uint32_t) (free_idx + 1)); + + return 0; +} + +/* Detach an injection point by flipping its generation back to even. */ +static int +do_detach(InjectionRegCtl *ctl, const char *name) +{ + uint32_t max_inuse = load_u32(&ctl->max_inuse); + + for (uint32_t i = 0; i < max_inuse; i++) + { + uint64_t g = load_u64(&ctl->entries[i].generation); + + if (g % 2 == 0) + continue; + if (strncmp(ctl->entries[i].name, name, INJ_NAME_MAXLEN) == 0) + { + store_u64_release(&ctl->entries[i].generation, g + 1); + return 0; + } + } + + fprintf(stderr, "%s: injection point \"%s\" not attached\n", + progname, name); + return 1; +} + +/* + * Return the wait slot currently registered for "name", or -1 if none. + * + * The backend writes the name under a spinlock before it starts waiting, so a + * stable match means the wait point has been reached. A torn read simply + * fails to match and the caller retries. + */ +static int +find_wait_slot(InjectionPointPublicState *state, const char *name) +{ + for (int i = 0; i < INJ_MAX_WAIT; i++) + { + if (strncmp(state->name[i], name, INJ_NAME_MAXLEN) == 0) + return i; + } + return -1; +} + +/* + * Poll the wait file until "name" appears (a process has reached the point), + * up to timeout_sec. Returns the slot, or -1 on timeout. + */ +static int +wait_for_slot(InjectionPointPublicState *state, const char *name, + int timeout_sec) +{ + int max_polls = (timeout_sec * 1000) / INJ_POLL_INTERVAL_MS; + + for (int polls = 0;; polls++) + { + int slot = find_wait_slot(state, name); + + if (slot >= 0) + return slot; + if (polls >= max_polls) + return -1; + sleep_ms(INJ_POLL_INTERVAL_MS); + } +} + +int +main(int argc, char **argv) +{ + const char *datadir; + const char *mode; + const char *name; + int timeout_sec = INJ_DEFAULT_TIMEOUT_SEC; + char path[1024]; + + if (argc < 4 || argc > 5) + { + usage(); + return 1; + } + + datadir = argv[1]; + mode = argv[2]; + name = argv[3]; + if (argc == 5) + timeout_sec = atoi(argv[4]); + + if (strlen(name) >= INJ_NAME_MAXLEN) + { + fprintf(stderr, "%s: injection point name too long\n", progname); + return 1; + } + + /* attach/detach operate on the core registry file. */ + if (strcmp(mode, "attach") == 0 || strcmp(mode, "detach") == 0) + { + InjectionRegCtl *ctl; + + snprintf(path, sizeof(path), "%s/%s", datadir, INJ_POINTS_FILE); + ctl = (InjectionRegCtl *) map_file(path, sizeof(InjectionRegCtl)); + if (ctl == NULL) + return 1; + + if (strcmp(mode, "attach") == 0) + return do_attach(ctl, name); + else + return do_detach(ctl, name); + } + + /* wait/wakeup operate on this module's wait file. */ + if (strcmp(mode, "wait") == 0 || strcmp(mode, "wakeup") == 0) + { + InjectionPointPublicState *state; + int slot; + + snprintf(path, sizeof(path), "%s/%s", datadir, INJ_STATE_FILE); + state = (InjectionPointPublicState *) map_file(path, + sizeof(InjectionPointPublicState)); + if (state == NULL) + return 1; + + slot = wait_for_slot(state, name, timeout_sec); + if (slot < 0) + { + fprintf(stderr, "%s: timed out waiting for injection point \"%s\"\n", + progname, name); + return 2; + } + + if (strcmp(mode, "wakeup") == 0) + atomic_inc_u32(&state->wait_counts[slot]); + + return 0; + } + + usage(); + return 1; +} diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build index 59dba1cb023..605dabb3ee8 100644 --- a/src/test/modules/injection_points/meson.build +++ b/src/test/modules/injection_points/meson.build @@ -21,6 +21,17 @@ injection_points = shared_module('injection_points', ) test_install_libs += injection_points +# Standalone client used by the TAP tests to wait on and wake up injection +# points without a backend connection. It only needs the module's own header. +injection_points_state = executable('injection_points_state', + files('injection_points_state.c'), + include_directories: include_directories('.'), + kwargs: default_bin_args + { + 'install': false, + }, +) +testprep_targets += injection_points_state + test_install_data += files( 'injection_points.control', 'injection_points--1.0.sql', @@ -30,6 +41,16 @@ tests += { 'name': 'injection_points', 'sd': meson.current_source_dir(), 'bd': meson.current_build_dir(), + 'tap': { + 'env': { + 'enable_injection_points': get_option('injection_points') ? 'yes' : 'no', + 'INJECTION_POINTS_STATE': injection_points_state.full_path(), + }, + 'tests': [ + 't/001_wait_without_sql.pl', + ], + 'deps': [injection_points_state], + }, 'regress': { 'sql': [ 'injection_points', diff --git a/src/test/modules/injection_points/t/001_wait_without_sql.pl b/src/test/modules/injection_points/t/001_wait_without_sql.pl new file mode 100644 index 00000000000..9a1cce1c1cf --- /dev/null +++ b/src/test/modules/injection_points/t/001_wait_without_sql.pl @@ -0,0 +1,128 @@ +# Copyright (c) 2026, PostgreSQL Global Development Group + +# Drive an injection point entirely from outside the server, without issuing +# any SQL to attach or coordinate it. Two files in the data directory back the +# shared state: +# +# - injection_points.shm: the core registry of attached points (see +# src/backend/utils/misc/injection_point.c). Writing it attaches a point. +# - injection_points_wait.shm: this module's wait/wakeup coordination (see +# injection_points.c). +# +# The standalone injection_points_state client maps these files the same way +# the backend does and is able to attach a "wait" point, detect that a process +# reached it, release it, and detach it -- all without a backend connection. +# This is the synchronization primitive needed when the cooperating process +# has no PGPROC or no wait-event visibility (postmaster, early startup, ...), +# where SQL-driven attach/wakeup is not available and a fixed sleep would be +# unreliable. SQL is used here only to *trigger* the point (the code path that +# would normally contain the INJECTION_POINT() macro) and to observe state. + +use strict; +use warnings FATAL => 'all'; + +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; +use Test::More; + +if ($ENV{enable_injection_points} ne 'yes') +{ + plan skip_all => 'Injection points not supported by this build'; +} + +my $client = $ENV{INJECTION_POINTS_STATE}; +if (!defined $client || $client eq '') +{ + plan skip_all => 'injection_points_state client not available'; +} + +# Preload the module so the wait file is created at startup and the library is +# available in every backend. +my $node = PostgreSQL::Test::Cluster->new('main'); +$node->init; +$node->append_conf('postgresql.conf', + "shared_preload_libraries = 'injection_points'"); +$node->start; +$node->safe_psql('postgres', 'CREATE EXTENSION injection_points;'); + +my $datadir = $node->data_dir; + +# Both backing files must exist as soon as the server is up. +ok(-f "$datadir/injection_points.shm", + 'core registry file created at startup'); +ok(-f "$datadir/injection_points_wait.shm", + 'wait state file created at startup'); + +# Attach a "wait" injection point by writing the registry file directly, with +# no SQL involved. +$node->command_ok( + [ $client, $datadir, 'attach', 'external-wait' ], + 'external client attached a wait point without SQL'); + +# The backend must see the externally-attached point in its registry. +my $listed = $node->safe_psql('postgres', + "SELECT point_name || ',' || library || ',' || function" + . " FROM injection_points_list() WHERE point_name = 'external-wait';"); +is( $listed, + 'external-wait,injection_points,injection_wait', + 'backend sees the externally-attached injection point'); + +# Trigger the point from a background session, which blocks in injection_wait(). +my $session = $node->background_psql('postgres', on_error_stop => 0); +$session->query_until( + qr/start/, qq[ + \\echo start + SELECT injection_points_run('external-wait'); +]); + +# Detect that the wait point was reached. The client polls the mapped wait +# file instead of guessing with a sleep, so it behaves the same on fast and +# slow machines. +$node->command_ok( + [ $client, $datadir, 'wait', 'external-wait' ], + 'external client detected the wait point without SQL'); + +# Detach the point *before* waking the waiter. In a code path that runs +# INJECTION_POINT() in a loop, waking first would let the woken process loop +# back and immediately re-enter the wait at the same still-attached point, so +# the robust order is detach-then-wake. Here the point is run once so the +# order is not strictly required, but the test models the correct pattern. +# Detach only flips the registry generation; the waiter stays blocked on the +# separate wait file until we bump its counter below. +$node->command_ok( + [ $client, $datadir, 'detach', 'external-wait' ], + 'external client detached the point without SQL'); + +# The backend must no longer see it in the registry, even though a process is +# still blocked at the point. +my $still = $node->safe_psql('postgres', + "SELECT count(*) FROM injection_points_list()" + . " WHERE point_name = 'external-wait';"); +is($still, '0', 'backend no longer sees the detached injection point'); + +# Release the waiter by bumping its counter through the mapped wait file, again +# without any SQL or backend connection. +$node->command_ok( + [ $client, $datadir, 'wakeup', 'external-wait' ], + 'external client woke the waiter without SQL'); + +# The blocked SELECT must now finish. +$session->query_safe('SELECT 1;'); +$session->quit; + +# A wait for the now-cleared point must time out rather than block forever, +# proving the tool reflects live state. +$node->command_fails_like( + [ $client, $datadir, 'wait', 'external-wait', '1' ], + qr/timed out/, + 'external client times out when no process waits'); + +$node->stop; + +# Both backing files are removed together with the cluster. +ok(!-f "$datadir/injection_points.shm", + 'core registry file removed at shutdown'); +ok(!-f "$datadir/injection_points_wait.shm", + 'wait state file removed at shutdown'); + +done_testing(); -- 2.50.1 (Apple Git-155) [application/octet-stream] v2026-06-14-0002-injection_points-back-the-active-points-.patch (15.5K, ../../[email protected]/4-v2026-06-14-0002-injection_points-back-the-active-points-.patch) download | inline diff: From 51b971709c3027388a9ccd17129f31be468dade8 Mon Sep 17 00:00:00 2001 From: Andrey Borodin <[email protected]> Date: Sat, 13 Jun 2026 13:05:22 +0500 Subject: [PATCH v2026-06-14 2/3] injection_points: back the active points array with a file Map ActiveInjectionPoints from a file in the data directory (injection_points.shm, overridable with PG_INJECTION_POINTS_FILE) instead of the main shared memory segment, so out-of-process tools can read and attach injection points with no backend connection. The lock-free generation protocol used to read the array is unchanged. EXEC_BACKEND children map the file themselves instead of inheriting the postmaster's pointer, which would address an arbitrary mapping the re-exec'd child does not reproduce. The backing file is anchored at the data directory (children attach before chdir'ing into it). The registry is mapped lazily on the first lookup, so a point that fires before this process attached -- an early child point such as "backend-initialize", or one armed out of band -- is still seen; if no file exists, nothing has been armed. --- src/backend/postmaster/launch_backend.c | 11 - src/backend/utils/misc/injection_point.c | 349 ++++++++++++++++++++++- 2 files changed, 335 insertions(+), 25 deletions(-) diff --git a/src/backend/postmaster/launch_backend.c b/src/backend/postmaster/launch_backend.c index 8f3cfea880c..14cceff54ef 100644 --- a/src/backend/postmaster/launch_backend.c +++ b/src/backend/postmaster/launch_backend.c @@ -99,9 +99,6 @@ typedef struct HANDLE UsedShmemSegID; #endif void *UsedShmemSegAddr; -#ifdef USE_INJECTION_POINTS - struct InjectionPointsCtl *ActiveInjectionPoints; -#endif PROC_HDR *ProcGlobal; PGPROC *AuxiliaryProcs; PGPROC *PreparedXactProcs; @@ -730,10 +727,6 @@ save_backend_variables(BackendParameters *param, param->UsedShmemSegID = UsedShmemSegID; param->UsedShmemSegAddr = UsedShmemSegAddr; -#ifdef USE_INJECTION_POINTS - param->ActiveInjectionPoints = ActiveInjectionPoints; -#endif - param->ProcGlobal = ProcGlobal; param->AuxiliaryProcs = AuxiliaryProcs; param->PreparedXactProcs = PreparedXactProcs; @@ -986,10 +979,6 @@ restore_backend_variables(BackendParameters *param) UsedShmemSegID = param->UsedShmemSegID; UsedShmemSegAddr = param->UsedShmemSegAddr; -#ifdef USE_INJECTION_POINTS - ActiveInjectionPoints = param->ActiveInjectionPoints; -#endif - ProcGlobal = param->ProcGlobal; AuxiliaryProcs = param->AuxiliaryProcs; PreparedXactProcs = param->PreparedXactProcs; diff --git a/src/backend/utils/misc/injection_point.c b/src/backend/utils/misc/injection_point.c index 272ef5e578a..2bb4579e0de 100644 --- a/src/backend/utils/misc/injection_point.c +++ b/src/backend/utils/misc/injection_point.c @@ -21,11 +21,17 @@ #ifdef USE_INJECTION_POINTS +#include <fcntl.h> #include <sys/stat.h> +#include <unistd.h> +#ifndef WIN32 +#include <sys/mman.h> +#endif #include "fmgr.h" #include "miscadmin.h" #include "storage/fd.h" +#include "storage/ipc.h" #include "storage/lwlock.h" #include "storage/shmem.h" #include "storage/subsystems.h" @@ -72,6 +78,17 @@ typedef struct InjectionPointEntry char private_data[INJ_PRIVATE_MAXLEN]; } InjectionPointEntry; +/* + * The active points array is mapped from a file (see below) that out-of-process + * tools can read and write. Those tools cannot use the pg_atomic_* API, so + * they mirror this layout with plain integers; make sure the atomic types stay + * layout-compatible with their underlying width. + */ +StaticAssertDecl(sizeof(pg_atomic_uint32) == sizeof(uint32), + "pg_atomic_uint32 must be layout-compatible with uint32"); +StaticAssertDecl(sizeof(pg_atomic_uint64) == sizeof(uint64), + "pg_atomic_uint64 must be layout-compatible with uint64"); + #define MAX_INJECTION_POINTS 128 /* @@ -87,8 +104,39 @@ typedef struct InjectionPointsCtl InjectionPointEntry entries[MAX_INJECTION_POINTS]; } InjectionPointsCtl; +/* + * The 8-byte-aligned generation counter pushes the entries array to offset 8, + * past max_inuse and its padding. Out-of-process tools mirror this, so pin it + * here too: this offset is part of the on-file contract. + */ +StaticAssertDecl(offsetof(InjectionPointsCtl, entries) == 8, + "InjectionPointsCtl.entries must start at offset 8"); + NON_EXEC_STATIC InjectionPointsCtl *ActiveInjectionPoints; +/* + * Name of the file backing the active injection points array. + * + * Unlike the rest of shared memory, this array lives in an ordinary file so + * that out-of-process tools (with no backend connection, no SQL, and possibly + * running before or instead of the postmaster) can map the same bytes, attach + * injection points and coordinate with the processes that hit them. The path + * defaults to this name relative to the data directory, but can be overridden + * with the PG_INJECTION_POINTS_FILE environment variable so that points can be + * attached even before initdb has created a data directory (e.g. for + * single-user mode bootstrap). + */ +#define INJ_POINTS_FILE "injection_points.shm" +#define INJ_POINTS_FILE_ENV "PG_INJECTION_POINTS_FILE" + +/* How injection_map_points() should open the backing file. */ +typedef enum InjectionMapMode +{ + INJ_MAP_ATTACH, /* map an already-existing file */ + INJ_MAP_ATTACH_OR_CREATE, /* attach if present, else create */ + INJ_MAP_ATTACH_IF_EXISTS, /* attach if present, else leave unmapped */ +} InjectionMapMode; + /* * Backend local cache of injection callbacks already loaded, stored in * TopMemoryContext. @@ -110,8 +158,9 @@ typedef struct InjectionPointCacheEntry static HTAB *InjectionPointCache = NULL; -static void InjectionPointShmemRequest(void *arg); -static void InjectionPointShmemInit(void *arg); +static void injection_shmem_init(void *arg); +static void injection_shmem_attach(void *arg); +static void injection_map_points(InjectionMapMode mode, int elevel); /* * injection_point_cache_add @@ -229,29 +278,281 @@ injection_point_cache_get(const char *name) return NULL; } +/* + * The active injection points array is backed by a file rather than the main + * shared memory segment, so we do not reserve any space there. init_fn maps + * (and, if needed, creates) the file once in the postmaster; children inherit + * the mapping through fork(), while attach_fn re-maps it in children that do + * not (EXEC_BACKEND/Windows). + */ const ShmemCallbacks InjectionPointShmemCallbacks = { - .request_fn = InjectionPointShmemRequest, - .init_fn = InjectionPointShmemInit, + .init_fn = injection_shmem_init, + .attach_fn = injection_shmem_attach, }; /* - * Reserve space for the dynamic shared hash table + * Resolve the path of the backing file. The result is cached in a static + * buffer so that the cleanup callback can unlink the same path that was + * mapped, regardless of later CWD or environment changes. */ -static void -InjectionPointShmemRequest(void *arg) +static const char * +injection_points_file_path(void) { - ShmemRequestStruct(.name = "InjectionPoint hash", - .size = sizeof(InjectionPointsCtl), - .ptr = (void **) &ActiveInjectionPoints, - ); + static char path[MAXPGPATH]; + const char *env; + + if (path[0] != '\0') + return path; + + env = getenv(INJ_POINTS_FILE_ENV); + if (env != NULL && env[0] != '\0') + strlcpy(path, env, sizeof(path)); + else if (DataDir != NULL && DataDir[0] != '\0') + { + /* + * Anchor the file at the data directory rather than relying on the + * current working directory. EXEC_BACKEND children re-map the file + * from attach_fn before they chdir into the data directory, so a + * relative path would resolve against the wrong directory there and + * the attach would fail. This also matches the absolute path that + * out-of-process tools build from the data directory they are given. + */ + snprintf(path, sizeof(path), "%s/%s", DataDir, INJ_POINTS_FILE); + } + else + strlcpy(path, INJ_POINTS_FILE, sizeof(path)); + + return path; } +/* + * Initialize a freshly-created array. + */ static void -InjectionPointShmemInit(void *arg) +injection_points_init_ctl(InjectionPointsCtl *ctl) { - pg_atomic_init_u32(&ActiveInjectionPoints->max_inuse, 0); + pg_atomic_init_u32(&ctl->max_inuse, 0); for (int i = 0; i < MAX_INJECTION_POINTS; i++) - pg_atomic_init_u64(&ActiveInjectionPoints->entries[i].generation, 0); + pg_atomic_init_u64(&ctl->entries[i].generation, 0); +} + +/* + * proc_exit callback that drops the mapping and, in the process that owns the + * cluster lifecycle (the postmaster, or a standalone backend), unlinks the + * backing file so it does not survive the cluster. Forked children inherit + * this callback but must not remove the file. + */ +static void +injection_points_file_cleanup(int code, Datum arg) +{ + if (ActiveInjectionPoints != NULL) + { +#ifndef WIN32 + munmap(ActiveInjectionPoints, sizeof(InjectionPointsCtl)); +#else + UnmapViewOfFile(ActiveInjectionPoints); +#endif + ActiveInjectionPoints = NULL; + } + + if (!IsUnderPostmaster) + (void) unlink(injection_points_file_path()); +} + +#ifndef WIN32 +/* + * Wait for a file created by a concurrent process to reach its final size. + * + * The winner of the O_EXCL create race makes the file at length zero and only + * then ftruncate()s it to "size". A process that lost the race and is + * attaching could otherwise mmap() past end-of-file and take a SIGBUS on first + * access. The gap is just the few instructions between create and ftruncate, + * so in practice this returns on the first check. + */ +static void +injection_wait_for_size(int fd, Size size, const char *path, int elevel) +{ + /* Generous bound; the writer needs only microseconds. */ + for (int i = 0; i < 10000; i++) + { + struct stat st; + + if (fstat(fd, &st) != 0) + { + ereport(elevel, + (errcode_for_file_access(), + errmsg("could not stat injection point file \"%s\": %m", + path))); + return; + } + if (st.st_size >= (off_t) size) + return; + pg_usleep(1000L); /* 1ms */ + } + + ereport(elevel, + (errmsg("injection point file \"%s\" was never sized by its creator", + path))); +} +#endif + +/* + * Map the backing file into this process, creating and/or initializing it as + * dictated by "mode", and set ActiveInjectionPoints. + * + * Accessors must always use the mapping, never plain file reads, because file + * I/O is not guaranteed to be coherent with a mapped view on Windows. + */ +static void +injection_map_points(InjectionMapMode mode, int elevel) +{ + const char *path = injection_points_file_path(); + Size size = sizeof(InjectionPointsCtl); + bool created = false; + + if (ActiveInjectionPoints != NULL) + return; + +#ifndef WIN32 + { + int fd; + int oflags = O_RDWR; + + if (mode == INJ_MAP_ATTACH_OR_CREATE) + oflags |= O_CREAT | O_EXCL; + + fd = OpenTransientFile(path, oflags); + if (fd < 0 && mode == INJ_MAP_ATTACH_OR_CREATE && errno == EEXIST) + { + /* Lost the race to create it; just attach. */ + oflags = O_RDWR; + fd = OpenTransientFile(path, oflags); + + /* + * The winner may not have ftruncate()d the file to full size yet; + * wait for it so the mmap() below cannot fault past end-of-file. + */ + if (fd >= 0) + injection_wait_for_size(fd, size, path, elevel); + } + else if (fd >= 0 && (oflags & O_CREAT)) + created = true; + + if (fd < 0) + { + /* A missing file simply means nothing has been armed yet. */ + if (mode == INJ_MAP_ATTACH_IF_EXISTS && errno == ENOENT) + return; + ereport(elevel, + (errcode_for_file_access(), + errmsg("could not open injection point file \"%s\": %m", + path))); + } + + if (created && ftruncate(fd, size) != 0) + { + CloseTransientFile(fd); + (void) unlink(path); + ereport(elevel, + (errcode_for_file_access(), + errmsg("could not size injection point file \"%s\": %m", + path))); + } + + ActiveInjectionPoints = mmap(NULL, size, PROT_READ | PROT_WRITE, + MAP_SHARED, fd, 0); + CloseTransientFile(fd); + + if (ActiveInjectionPoints == MAP_FAILED) + { + ActiveInjectionPoints = NULL; + ereport(elevel, + (errcode_for_file_access(), + errmsg("could not map injection point file \"%s\": %m", + path))); + } + } +#else + { + HANDLE hfile; + HANDLE hmap; + DWORD share = FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE; + DWORD disp = (mode == INJ_MAP_ATTACH_OR_CREATE) ? CREATE_NEW : OPEN_EXISTING; + + hfile = CreateFile(path, + GENERIC_READ | GENERIC_WRITE, + share, NULL, disp, FILE_ATTRIBUTE_NORMAL, NULL); + + /* + * If creation failed because a file from an earlier cluster lifetime + * is in the way, attach to it instead of failing to start. Do not + * insist on a specific error code: besides ERROR_FILE_EXISTS, a file + * whose deletion is still pending reports ERROR_ACCESS_DENIED, and we + * want to recover from that too. FILE_SHARE_DELETE (above) lets such a + * file be reopened and lets the cleanup unlink it while still mapped. + */ + if (hfile == INVALID_HANDLE_VALUE && mode == INJ_MAP_ATTACH_OR_CREATE) + { + disp = OPEN_EXISTING; + hfile = CreateFile(path, + GENERIC_READ | GENERIC_WRITE, + share, NULL, disp, FILE_ATTRIBUTE_NORMAL, NULL); + } + else if (hfile != INVALID_HANDLE_VALUE && disp == CREATE_NEW) + created = true; + + if (hfile == INVALID_HANDLE_VALUE) + { + DWORD err = GetLastError(); + + /* A missing file simply means nothing has been armed yet. */ + if (mode == INJ_MAP_ATTACH_IF_EXISTS && + (err == ERROR_FILE_NOT_FOUND || err == ERROR_PATH_NOT_FOUND)) + return; + ereport(elevel, + (errmsg("could not open injection point file \"%s\": error code %lu", + path, err))); + } + + /* CreateFileMapping extends the backing file to the mapping size. */ + hmap = CreateFileMapping(hfile, NULL, PAGE_READWRITE, 0, + (DWORD) size, NULL); + if (hmap == NULL) + { + CloseHandle(hfile); + ereport(elevel, + (errmsg("could not create mapping for injection point file \"%s\": error code %lu", + path, GetLastError()))); + } + + ActiveInjectionPoints = MapViewOfFile(hmap, FILE_MAP_ALL_ACCESS, 0, 0, size); + CloseHandle(hmap); + CloseHandle(hfile); + + if (ActiveInjectionPoints == NULL) + ereport(elevel, + (errmsg("could not map injection point file \"%s\": error code %lu", + path, GetLastError()))); + } +#endif + + if (created) + { + injection_points_init_ctl(ActiveInjectionPoints); + on_proc_exit(injection_points_file_cleanup, 0); + } +} + +static void +injection_shmem_init(void *arg) +{ + injection_map_points(INJ_MAP_ATTACH_OR_CREATE, FATAL); +} + +static void +injection_shmem_attach(void *arg) +{ + injection_map_points(INJ_MAP_ATTACH, FATAL); } #endif /* USE_INJECTION_POINTS */ @@ -412,6 +713,26 @@ InjectionPointCacheRefresh(const char *name) InjectionPointEntry local_copy; InjectionPointCacheEntry *cached; + /* + * The registry may not be mapped yet in this process: an EXEC_BACKEND + * child reaches early points (e.g. "backend-initialize") before its shmem + * attach callback runs, and the postmaster itself runs points before it + * creates the file. Map it lazily so a point armed out of band, or before + * this process attached, is never missed. If no file exists nothing has + * been armed; a genuine mapping failure is reported rather than hidden. + */ + if (ActiveInjectionPoints == NULL) + injection_map_points(INJ_MAP_ATTACH_IF_EXISTS, ERROR); + if (ActiveInjectionPoints == NULL) + { + if (InjectionPointCache) + { + hash_destroy(InjectionPointCache); + InjectionPointCache = NULL; + } + return NULL; + } + /* * First read the number of in-use slots. More entries can be added or * existing ones can be removed while we're reading them. If the entry -- 2.50.1 (Apple Git-155) ^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: injection_points: Switch wait/wakeup to use atomics rather than latches @ 2026-06-15 00:14 Michael Paquier <[email protected]> parent: Andrey Borodin <[email protected]> 0 siblings, 1 reply; 21+ messages in thread From: Michael Paquier @ 2026-06-15 00:14 UTC (permalink / raw) To: Andrey Borodin <[email protected]>; +Cc: Robert Haas <[email protected]>; Postgres hackers <[email protected]>; Heikki Linnakangas <[email protected]> On Sun, Jun 14, 2026 at 02:23:54PM +0500, Andrey Borodin wrote: > The same revision also closes an unrelated init race: when two processes > create the backing file at once, the loser could map it before the winner > sized it (SIG-something on first touch). The attach path now waits for the > file to reach full size before mapping. > > CI is green. Same two patches on top of your atomics commit. Let me know > if this prototype goes radically wrong way. You may find my reply surprising (or not), but using a client tool to bypass the SQL protocol is super invasive in terms of the in-core changes, and I'm -1 on that. Test tooling should rely on simple facilities, and you are re-implementing quite a few things here that come with a new class of bugs and what look like design issues to me due to the invasiveness: - Reimplementation of the registry protocol for file mapping. That may be useful for other things, and there may be use for a refactored in-core API, but I don't see why we should use it here: - Mirroring of internal structs for shmem manipulation. - Core data updates with zero locking. I am wondering if we are not overcomplicating things here.. How about this idea instead of 0002 and 0003, for paths that cannot rely on some SQL: - Let's use a file-based markup, located in a injection_points/ in the data folder. - When attaching a wait point, write a marker, say wait_$POINT. - On wakeup, write a wakeup_$POINT. - The wait routine does periodic checks of the wakeup_$POINT file, using a stat(). It depends on how much we want to achieve, but forcing a postmaster to wait at an early startup sequence would then be something like: - Add the a wait markup. - At startup, shared_preload_libraries scans the pg_injection_points/ repo, fills in its shmem state by calling InjectionPointAttach(). - postmaster hits the injection point, calls injection_wait() - Test does what it wants while the postmaster waits, manipulates states. - Test script drops a wakeup file. - Postmaster sees the file, continues its startup sequence. This means one cannot set an injection point until shared_preload_libraries is loaded, of course. Cleanup logic feels kind of nice: test cleans his stuff, or just remove pg_injection_points/ at shutdown with an exit callback. No platform specific logic, only FS checks. This means a bit higher latency, but it eliminates a full class of bugs due to the fact that it is simpler. This would need to live alongside patch 0001 so as a _PG_init() can be loaded when the injection_points lib is loaded; we're still going to need it to fill the shmem info with the wait slot being occupied. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: injection_points: Switch wait/wakeup to use atomics rather than latches @ 2026-07-07 09:51 Andrey Borodin <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 1 reply; 21+ messages in thread From: Andrey Borodin @ 2026-07-07 09:51 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Robert Haas <[email protected]>; Postgres hackers <[email protected]>; Heikki Linnakangas <[email protected]> Hi Michael, You suggested driving the module's wait points through the filesystem instead of mapping shared memory out of process. Here is that, rebased on your atomics commit. It is a test-module-only change; the core injection point registry is untouched. This message and the patch is AI-editorialized for readability. Design ------ Control dirs and files live under pg_injection_points/ in the data directory: pg_injection_points/<point>/ attaches <point> as a wait point pg_injection_points/<point>/<pid> that backend is waiting at <point> The module scans this directory once at postmaster startup and attaches a wait point for each subdirectory, so a test can arm a point before the server is up (shared_preload_libraries is required for that). When a process reaches the point it creates its <pid> file and waits, polling with stat() until the file is removed, in addition to the existing wakeup counter. So an out-of-process test can: - arm a point with mkdir, before SQL is available; - see which backends are blocked by listing the directory; - wake one specific backend by unlinking its <pid> file. No SQL connection, no platform-specific code, and no out-of-process access to shared memory. The scan is deliberately one-shot at startup. If a consumer ever needs to (re)load points from the filesystem at runtime, that can be added later as a dedicated injection point action (say a "reload" type); I think we grow the facility only as the tests actually require. The test question ----------------- But in this project we grow the facility only when a test actually needs it, so the real question is which test justifies this one. The first case that seems to call for it is the ProcKill lock-group / procLatch recycle race [0]. On that thread you concluded that a wait point inside ProcKill() cannot use the latch-based wait, because the fix (84b9d6bceab6) now disowns the latch earlier, and that the test had to lean on statement_timeout to keep the leader parked long enough - which is an anti-pattern. You (IDK, maybe someone else) suggested switching the wait to a latch-free shmem flag on HEAD; that is your atomics commit, and this patch builds on it. What the filesystem adds on top is the observability that was still awkward there. Instead of statement_timeout and an unreliable pg_stat_activity (or query_until banners a la 011_lock_stats.pl), the controller just waits for pg_injection_points/<point>/<pid> to appear and then wakes that exact PID. It also sidesteps the before_shmem_exit(injection_points_cleanup) hook that detaches a self-attached point before ProcKill runs, since the point is armed by the startup scan rather than by the victim. So the question is whether to rewrite that reproducer on top of these two patches. This design goes far beyond the minimal plan you sketched (latch-free wait, attach() with an optional PID, then the test), and that you were not sure the ProcKill case alone justifies the churn. Before writing it I would confirm it really needs the filesystem parts and is not better served by something simpler. Alternatively, we can use new capability for some other tests. WDYT? Thank you! Best regards, Andrey Borodin. [0] https://www.postgresql.org/message-id/[email protected] Attachments: [application/octet-stream] v2026-07-07-0002-injection_points-drive-wait-points-throu.patch (12.8K, ../../[email protected]/2-v2026-07-07-0002-injection_points-drive-wait-points-throu.patch) download | inline diff: From 8c9712d9dfcff54804781b95cd55675eb2688206 Mon Sep 17 00:00:00 2001 From: Andrey Borodin <[email protected]> Date: Tue, 16 Jun 2026 15:17:28 +0500 Subject: [PATCH v2026-07-07 2/2] injection_points: drive wait points through the filesystem, without SQL Add a filesystem-based way to attach a wait point and release a specific waiter, for code paths that run before the server can answer SQL (e.g. early postmaster startup). It works alongside the SQL path and does not touch the core registry. State lives under pg_injection_points/ in the data directory: pg_injection_points/<point>/ present -> <point> attached as a wait pg_injection_points/<point>/<pid> present -> that backend is parked here Each subdirectory is scanned once at startup and attached, so shared_preload_libraries is needed to arm a point before startup. A backend reaching the point publishes its <pid> file and polls with stat() until it is removed, besides watching the wakeup counter; removing the file wakes that one backend, and listing the directory shows which backends are blocked. The tree is dropped at shutdown. --- src/test/modules/injection_points/Makefile | 2 + .../injection_points/injection_points.c | 141 +++++++++++++++++- src/test/modules/injection_points/meson.build | 8 + .../t/001_wait_without_sql.pl | 86 +++++++++++ 4 files changed, 234 insertions(+), 3 deletions(-) create mode 100644 src/test/modules/injection_points/t/001_wait_without_sql.pl diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile index c01d2fb095c..3aca8c389de 100644 --- a/src/test/modules/injection_points/Makefile +++ b/src/test/modules/injection_points/Makefile @@ -24,6 +24,8 @@ ISOLATION = basic \ # some isolation tests require wal_level=replica ISOLATION_OPTS = --temp-config $(top_srcdir)/src/test/modules/injection_points/extra.conf +TAP_TESTS = 1 + # The injection points are cluster-wide, so disable installcheck NO_INSTALLCHECK = 1 diff --git a/src/test/modules/injection_points/injection_points.c b/src/test/modules/injection_points/injection_points.c index 9b8e1aaad0b..d0a7d987250 100644 --- a/src/test/modules/injection_points/injection_points.c +++ b/src/test/modules/injection_points/injection_points.c @@ -17,6 +17,10 @@ #include "postgres.h" +#include <fcntl.h> +#include <sys/stat.h> + +#include "common/file_utils.h" #include "fmgr.h" #include "funcapi.h" #include "injection_points.h" @@ -24,6 +28,7 @@ #include "nodes/pg_list.h" #include "nodes/value.h" #include "storage/dsm_registry.h" +#include "storage/fd.h" #include "storage/ipc.h" #include "storage/lwlock.h" #include "storage/shmem.h" @@ -41,6 +46,39 @@ PG_MODULE_MAGIC; #define INJ_MAX_WAIT 8 #define INJ_NAME_MAXLEN 64 +/* + * Filesystem markers used to drive wait points without any SQL connection, for + * code paths that run before the server can answer SQL (e.g. early postmaster + * startup). They live under a directory in the data directory, one + * subdirectory per attached point and one file per parked process: + * + * pg_injection_points/<point>/ present = <point> attached as a wait; + * pg_injection_points/<point>/<pid> present = that process is parked here. + * + * A test attaches a point by creating its directory (scanned at startup), sees + * who is waiting by listing the directory, and wakes a specific waiter by + * removing its PID file. Everything is plain filesystem state checked with + * stat(), so there is no platform-specific logic and no out-of-process access + * to shared memory. Paths are relative to the data directory, which is the + * working directory of the postmaster and every backend. + */ +#define INJ_POINTS_DIR "pg_injection_points" + +/* "pg_injection_points/<point>" */ +static void +injection_point_dir(char *buf, size_t bufsize, const char *name) +{ + snprintf(buf, bufsize, "%s/%s", INJ_POINTS_DIR, name); +} + +/* "pg_injection_points/<point>/<pid>" */ +static void +injection_point_waiter_path(char *buf, size_t bufsize, + const char *name, int pid) +{ + snprintf(buf, bufsize, "%s/%s/%d", INJ_POINTS_DIR, name, pid); +} + /* * List of injection points stored in TopMemoryContext attached * locally to this process. @@ -113,6 +151,60 @@ injection_shmem_request(void *arg) ); } +/* + * Scan INJ_POINTS_DIR for point subdirectories and attach each as a wait point. + * Run once at postmaster startup so the points are in place before any process + * - the postmaster's own startup sequence or its children - reaches them, all + * without an SQL connection. A point can therefore be attached, by creating + * its directory, before the server is started. + */ +static void +injection_points_preload(void) +{ + DIR *dir; + struct dirent *de; + + dir = AllocateDir(INJ_POINTS_DIR); + if (dir == NULL) + return; /* no directory means nothing attached */ + + while ((de = ReadDir(dir, INJ_POINTS_DIR)) != NULL) + { + InjectionPointCondition condition = {.type = INJ_CONDITION_ALWAYS}; + char path[MAXPGPATH]; + struct stat st; + + if (strcmp(de->d_name, ".") == 0 || strcmp(de->d_name, "..") == 0) + continue; + if (strlen(de->d_name) >= INJ_NAME_MAXLEN) + continue; + + /* Each subdirectory names a wait point to attach. */ + injection_point_dir(path, sizeof(path), de->d_name); + if (stat(path, &st) != 0 || !S_ISDIR(st.st_mode)) + continue; + + InjectionPointAttach(de->d_name, "injection_points", "injection_wait", + &condition, sizeof(condition)); + } + FreeDir(dir); +} + +/* + * proc_exit callback that removes INJ_POINTS_DIR so markers do not survive the + * cluster. Forked children inherit this callback but must not run it, hence + * the IsUnderPostmaster guard: only the lifecycle owner (postmaster, or a + * standalone backend) cleans up. + */ +static void +injection_points_dir_cleanup(int code, Datum arg) +{ + struct stat st; + + if (!IsUnderPostmaster && stat(INJ_POINTS_DIR, &st) == 0) + (void) rmtree(INJ_POINTS_DIR, true); +} + static void injection_shmem_init(void *arg) { @@ -121,6 +213,13 @@ injection_shmem_init(void *arg) * initialization using a DSM. */ injection_point_init_state(inj_state, NULL); + + /* + * Attach any wait points requested out of band through marker directories, + * and make sure the directory does not outlive the cluster. + */ + injection_points_preload(); + on_proc_exit(injection_points_dir_cleanup, 0); } /* @@ -219,7 +318,7 @@ injection_notice(const char *name, const void *private_data, void *arg) elog(NOTICE, "notice triggered for injection point %s", name); } -/* Wait until injection_points_wakeup() is called */ +/* Wait until released by injection_points_wakeup() or a removed marker file */ void injection_wait(const char *name, const void *private_data, void *arg) { @@ -227,6 +326,8 @@ injection_wait(const char *name, const void *private_data, void *arg) int index = -1; uint32 injection_wait_event = 0; const InjectionPointCondition *condition = private_data; + char waiter_path[MAXPGPATH]; + bool have_waiter_file = false; if (inj_state == NULL) injection_init_shmem(); @@ -262,7 +363,35 @@ injection_wait(const char *name, const void *private_data, void *arg) name); /* - * Wait until the counter is bumped by injection_points_wakeup(). + * Publish our presence in the filesystem so an out-of-process test can see + * that we are parked here and release us with no SQL connection: create + * "pg_injection_points/<name>/<pid>" and wait until it is removed. The + * directories are created on demand, so this works for points attached + * through SQL too, not only those attached from a marker directory. + */ + { + char dirpath[MAXPGPATH]; + int fd; + + injection_point_dir(dirpath, sizeof(dirpath), name); + (void) MakePGDirectory(INJ_POINTS_DIR); + (void) MakePGDirectory(dirpath); + + injection_point_waiter_path(waiter_path, sizeof(waiter_path), name, + MyProcPid); + fd = OpenTransientFile(waiter_path, O_RDWR | O_CREAT | O_TRUNC); + if (fd >= 0) + { + CloseTransientFile(fd); + have_waiter_file = true; + } + } + + /* + * Wait until released, either by injection_points_wakeup() bumping the + * counter or by our waiter file being removed. The latter needs no SQL + * connection, so it works in code paths that run before the server can + * answer queries. * * This loop starts with a short delay for responsiveness, enlarged to * ease the CPU workload in slower environments. @@ -272,8 +401,10 @@ injection_wait(const char *name, const void *private_data, void *arg) pgstat_report_wait_start(injection_wait_event); { int delay_us = INJ_WAIT_INITIAL_US; + struct stat st; - while (pg_atomic_read_u32(&inj_state->wait_counts[index]) == old_wait_counts) + while (pg_atomic_read_u32(&inj_state->wait_counts[index]) == old_wait_counts && + (!have_waiter_file || stat(waiter_path, &st) == 0)) { CHECK_FOR_INTERRUPTS(); pg_usleep(delay_us); @@ -283,6 +414,10 @@ injection_wait(const char *name, const void *private_data, void *arg) } pgstat_report_wait_end(); + /* Clean up our waiter file; it may still exist after a counter wakeup. */ + if (have_waiter_file) + (void) unlink(waiter_path); + /* Remove this injection point from the waiters. */ SpinLockAcquire(&inj_state->lock); inj_state->name[index][0] = '\0'; diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build index 59dba1cb023..f6b53e1a33f 100644 --- a/src/test/modules/injection_points/meson.build +++ b/src/test/modules/injection_points/meson.build @@ -60,4 +60,12 @@ tests += { '--temp-config', files('extra.conf'), ], }, + 'tap': { + 'env': { + 'enable_injection_points': get_option('injection_points') ? 'yes' : 'no', + }, + 'tests': [ + 't/001_wait_without_sql.pl', + ], + }, } diff --git a/src/test/modules/injection_points/t/001_wait_without_sql.pl b/src/test/modules/injection_points/t/001_wait_without_sql.pl new file mode 100644 index 00000000000..8c33e99f24f --- /dev/null +++ b/src/test/modules/injection_points/t/001_wait_without_sql.pl @@ -0,0 +1,86 @@ +# Copyright (c) 2026, PostgreSQL Global Development Group + +# Exercise driving a wait injection point purely through the filesystem, with +# no SQL used for the coordination itself. This is meant for code paths that +# run before the server can answer queries (e.g. early postmaster startup). +# +# Layout under the data directory: +# pg_injection_points/<point>/ present -> <point> attached as a wait +# pg_injection_points/<point>/<pid> present -> that backend is parked here +# Removing the <pid> file wakes that backend. + +use strict; +use warnings FATAL => 'all'; + +use Time::HiRes qw(usleep); + +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; +use Test::More; + +if ($ENV{enable_injection_points} ne 'yes') +{ + plan skip_all => 'Injection points not supported by this build'; +} + +my $point = 'no-sql-wait'; + +my $node = PostgreSQL::Test::Cluster->new('node'); +$node->init; +$node->append_conf('postgresql.conf', + "shared_preload_libraries = 'injection_points'"); + +# Attach the wait point before the server is even running, without any SQL: +# just create its directory. The module scans these at startup. +my $root = $node->data_dir . '/pg_injection_points'; +my $pdir = "$root/$point"; +mkdir $root or die "could not create $root: $!"; +mkdir $pdir or die "could not create $pdir: $!"; + +$node->start; + +# The module attached the point from its directory at startup; we never called +# injection_points_attach(). +$node->safe_psql('postgres', 'CREATE EXTENSION injection_points;'); + +my $listed = $node->safe_psql('postgres', + "SELECT count(*) FROM injection_points_list() WHERE point_name = '$point';" +); +is($listed, '1', 'wait point attached from its directory, without SQL'); + +# Fire the point in a background session; it must block in injection_wait(). +my $bg = $node->background_psql('postgres'); +$bg->query_until( + qr/start/, qq[ +\\echo start +SELECT injection_points_run('$point'); +]); + +# Detect that the backend is parked, purely through the filesystem: it +# publishes a file named after its PID inside the point directory. +my $waiter; +foreach my $i (1 .. 1800) +{ + if (opendir(my $dh, $pdir)) + { + ($waiter) = grep { /^\d+\z/ } readdir($dh); + closedir($dh); + last if defined $waiter; + } + usleep(100_000); +} +ok(defined $waiter, 'backend published its waiter file (filesystem-observable)'); + +# Wake that specific backend without any SQL: remove its waiter file. +unlink "$pdir/$waiter" or die "could not remove waiter file: $!"; + +# The blocked statement now finishes, proving the filesystem wakeup worked. +like($bg->query_safe('SELECT 1;'), + qr/^1$/m, 'backend released by removing its waiter file, without SQL'); +$bg->quit; + +# The directory does not survive the cluster. +$node->stop; +ok(!-d $root, 'injection points directory removed at shutdown'); + +done_testing(); -- 2.50.1 (Apple Git-155) [application/octet-stream] v2026-07-07-0001-injection_points-Switch-wait-wakeup-to-r.patch (4.9K, ../../[email protected]/3-v2026-07-07-0001-injection_points-Switch-wait-wakeup-to-r.patch) download | inline diff: From 9eed8010addde7425d675efa123a4b167925ae08 Mon Sep 17 00:00:00 2001 From: Michael Paquier <[email protected]> Date: Thu, 28 May 2026 11:15:33 +0900 Subject: [PATCH v2026-07-07 1/2] injection_points: Switch wait/wakeup to rely on atomics This removes the dependency based on counters and environment variables, replacing the waiting loop by a wait on an atomic counter, whose check increases over time in an exponential manner (starts at 10us, up to 100ms). --- .../injection_points/injection_points.c | 53 ++++++++++--------- 1 file changed, 27 insertions(+), 26 deletions(-) diff --git a/src/test/modules/injection_points/injection_points.c b/src/test/modules/injection_points/injection_points.c index ba282e3dcab..9b8e1aaad0b 100644 --- a/src/test/modules/injection_points/injection_points.c +++ b/src/test/modules/injection_points/injection_points.c @@ -23,11 +23,11 @@ #include "miscadmin.h" #include "nodes/pg_list.h" #include "nodes/value.h" -#include "storage/condition_variable.h" #include "storage/dsm_registry.h" #include "storage/ipc.h" #include "storage/lwlock.h" #include "storage/shmem.h" +#include "storage/spin.h" #include "utils/builtins.h" #include "utils/guc.h" #include "utils/injection_point.h" @@ -59,13 +59,10 @@ typedef struct InjectionPointSharedState slock_t lock; /* Counters advancing when injection_points_wakeup() is called */ - uint32 wait_counts[INJ_MAX_WAIT]; + pg_atomic_uint32 wait_counts[INJ_MAX_WAIT]; /* Names of injection points attached to wait counters */ char name[INJ_MAX_WAIT][INJ_NAME_MAXLEN]; - - /* Condition variable used for waits and wakeups */ - ConditionVariable wait_point; } InjectionPointSharedState; /* Pointer to shared-memory state. */ @@ -102,9 +99,9 @@ injection_point_init_state(void *ptr, void *arg) InjectionPointSharedState *state = (InjectionPointSharedState *) ptr; SpinLockInit(&state->lock); - memset(state->wait_counts, 0, sizeof(state->wait_counts)); memset(state->name, 0, sizeof(state->name)); - ConditionVariableInit(&state->wait_point); + for (int i = 0; i < INJ_MAX_WAIT; i++) + pg_atomic_init_u32(&state->wait_counts[i], 0); } static void @@ -222,7 +219,7 @@ injection_notice(const char *name, const void *private_data, void *arg) elog(NOTICE, "notice triggered for injection point %s", name); } -/* Wait on a condition variable, awaken by injection_points_wakeup() */ +/* Wait until injection_points_wakeup() is called */ void injection_wait(const char *name, const void *private_data, void *arg) { @@ -254,31 +251,37 @@ injection_wait(const char *name, const void *private_data, void *arg) { index = i; strlcpy(inj_state->name[i], name, INJ_NAME_MAXLEN); - old_wait_counts = inj_state->wait_counts[i]; + old_wait_counts = pg_atomic_read_u32(&inj_state->wait_counts[i]); break; } } SpinLockRelease(&inj_state->lock); if (index < 0) - elog(ERROR, "could not find free slot for wait of injection point %s ", + elog(ERROR, "could not find free slot for wait of injection point %s", name); - /* And sleep.. */ - ConditionVariablePrepareToSleep(&inj_state->wait_point); - for (;;) + /* + * Wait until the counter is bumped by injection_points_wakeup(). + * + * This loop starts with a short delay for responsiveness, enlarged to + * ease the CPU workload in slower environments. + */ +#define INJ_WAIT_INITIAL_US 10 /* 10us */ +#define INJ_WAIT_MAX_US 100000 /* 100ms */ + pgstat_report_wait_start(injection_wait_event); { - uint32 new_wait_counts; + int delay_us = INJ_WAIT_INITIAL_US; - SpinLockAcquire(&inj_state->lock); - new_wait_counts = inj_state->wait_counts[index]; - SpinLockRelease(&inj_state->lock); - - if (old_wait_counts != new_wait_counts) - break; - ConditionVariableSleep(&inj_state->wait_point, injection_wait_event); + while (pg_atomic_read_u32(&inj_state->wait_counts[index]) == old_wait_counts) + { + CHECK_FOR_INTERRUPTS(); + pg_usleep(delay_us); + if (delay_us < INJ_WAIT_MAX_US) + delay_us *= 2; + } } - ConditionVariableCancelSleep(); + pgstat_report_wait_end(); /* Remove this injection point from the waiters. */ SpinLockAcquire(&inj_state->lock); @@ -443,7 +446,7 @@ injection_points_wakeup(PG_FUNCTION_ARGS) if (inj_state == NULL) injection_init_shmem(); - /* First bump the wait counter for the injection point to wake up */ + /* Find the injection point then bump its wait counter */ SpinLockAcquire(&inj_state->lock); for (int i = 0; i < INJ_MAX_WAIT; i++) { @@ -458,11 +461,9 @@ injection_points_wakeup(PG_FUNCTION_ARGS) SpinLockRelease(&inj_state->lock); elog(ERROR, "could not find injection point %s to wake up", name); } - inj_state->wait_counts[index]++; SpinLockRelease(&inj_state->lock); - /* And broadcast the change to the waiters */ - ConditionVariableBroadcast(&inj_state->wait_point); + pg_atomic_fetch_add_u32(&inj_state->wait_counts[index], 1); PG_RETURN_VOID(); } -- 2.50.1 (Apple Git-155) ^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: injection_points: Switch wait/wakeup to use atomics rather than latches @ 2026-07-08 06:29 Michael Paquier <[email protected]> parent: Andrey Borodin <[email protected]> 0 siblings, 1 reply; 21+ messages in thread From: Michael Paquier @ 2026-07-08 06:29 UTC (permalink / raw) To: Andrey Borodin <[email protected]>; +Cc: Robert Haas <[email protected]>; Postgres hackers <[email protected]>; Heikki Linnakangas <[email protected]> On Tue, Jul 07, 2026 at 02:51:04PM +0500, Andrey Borodin wrote: > So the question is whether to rewrite that reproducer on top of these > two patches. This design goes far beyond the minimal > plan you sketched (latch-free wait, attach() with an optional PID, then > the test), and that you were not sure the ProcKill case alone justifies > the churn. Before writing it I would confirm it really needs the > filesystem parts and is not better served by something simpler. > > Alternatively, we can use new capability for some other tests. > > WDYT? The hard part is that the implementation choices are driven by the needs, where we may want to be able to do the following things without having to touch SQL: - Register a wait() at very early stages. - Know from a client perspective that a PID is waiting, as we may not have access to pg_stat_activity. - Trigger a wakeup. Your patch is able to achieve all of that, but it may be better to know better about more use cases folks have seen before taking any hard decision. Here, one good case that I could see in the tree is 007_pre_auth.pl, that uses currently as a workaround a background connection to create a wait point. We could switch that to register a wait early, but the impact is limited. @Heikki, what kind of ideas did you have for some of your toy tests recently, particularly with the protocol area? The protocol tests now in the tree use errors (backend-initialize, GSSAPI and SSL startup points), not waits and/or wakeups. Applied the patch that removes the condition variable dependency, btw. that's one thing less to worry about. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: injection_points: Switch wait/wakeup to use atomics rather than latches @ 2026-07-08 07:17 Heikki Linnakangas <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 1 reply; 21+ messages in thread From: Heikki Linnakangas @ 2026-07-08 07:17 UTC (permalink / raw) To: Michael Paquier <[email protected]>; Andrey Borodin <[email protected]>; +Cc: Robert Haas <[email protected]>; Postgres hackers <[email protected]> On 08/07/2026 09:29, Michael Paquier wrote: > On Tue, Jul 07, 2026 at 02:51:04PM +0500, Andrey Borodin wrote: >> So the question is whether to rewrite that reproducer on top of these >> two patches. This design goes far beyond the minimal >> plan you sketched (latch-free wait, attach() with an optional PID, then >> the test), and that you were not sure the ProcKill case alone justifies >> the churn. Before writing it I would confirm it really needs the >> filesystem parts and is not better served by something simpler. >> >> Alternatively, we can use new capability for some other tests. >> >> WDYT? > > The hard part is that the implementation choices are driven by the > needs, where we may want to be able to do the following things without > having to touch SQL: > - Register a wait() at very early stages. > - Know from a client perspective that a PID is waiting, as we may not > have access to pg_stat_activity. > - Trigger a wakeup. > Your patch is able to achieve all of that, but it may be better to > know better about more use cases folks have seen before taking any > hard decision. > > Here, one good case that I could see in the tree is 007_pre_auth.pl, > that uses currently as a workaround a background connection to create > a wait point. We could switch that to register a wait early, but the > impact is limited. > > @Heikki, what kind of ideas did you have for some of your toy tests > recently, particularly with the protocol area? The protocol tests now > in the tree use errors (backend-initialize, GSSAPI and SSL startup > points), not waits and/or wakeups. I've got nothing in mind right now, but I remember when we started to talk about this, I was working on something where I wanted to inject waits before authentication. I think it was related to dead-end backends, max_connections, reserved_connections and all that. - Heikki ^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: injection_points: Switch wait/wakeup to use atomics rather than latches @ 2026-07-08 19:17 Andrey Borodin <[email protected]> parent: Heikki Linnakangas <[email protected]> 0 siblings, 0 replies; 21+ messages in thread From: Andrey Borodin @ 2026-07-08 19:17 UTC (permalink / raw) To: Heikki Linnakangas <[email protected]>; +Cc: Michael Paquier <[email protected]>; Robert Haas <[email protected]>; Postgres hackers <[email protected]> > On 8 Jul 2026, at 12:17, Heikki Linnakangas <[email protected]> wrote: > > dead-end backends, max_connections, reserved_connections and all that Hi Heikki, Michael, Thanks for pushing the atomics patch. Following Heikki's pointer to waits before authentication and the connection-limit area, here is where I looked: - 002_connection_limits.pl did not seem to need a blocking wait, closer to the WARNING/LOG point (as discussed on the postmaster-cleanup thread [0]). - 007_pre_auth.pl already attaches init-pre-auth, but it exists to test pg_stat_activity itself, and its old race (matching state='starting' before the backend reached the point) is already fixed by querying wait_event. So converting it buys little, as you noted. - So PFA a new test: a connection counts against max_connections from InitProcessPhase2(), before authentication. I added a TAP test that fills every slot with pre-auth backends and checks that one more connection is refused with "too many clients". Such backends can't be identified over SQL and there is no free slot for a monitoring session, so the test finds them through the filesystem markers. If this test direction seems viable - I'll polish the test. Is this enough to justify the filesystem layer, or would you rather see more cases? Heikki, if you have a specific dead-end-backend / reserved-connection scenario in mind, I'm happy to aim the test there. It would be slightly better to have an unfixed bug to close with such test. But I do not have a spare one, so of course we could introduce a new bug. But that clearly deserves its own thread. Thanks! Best regards, Andrey Borodin. [0] https://postgr.es/m/ggflhkciwdyotpoie323chu2c2idpjk5qimrn462encwx2io7s@thmcxl7i6dpw Attachments: [application/octet-stream] v2026-07-08-0001-injection_points-drive-wait-points-throu.patch (12.8K, ../../[email protected]/2-v2026-07-08-0001-injection_points-drive-wait-points-throu.patch) download | inline diff: From 890cb2be24e95c40da10ef2b5d380735fb020198 Mon Sep 17 00:00:00 2001 From: Andrey Borodin <[email protected]> Date: Tue, 16 Jun 2026 15:17:28 +0500 Subject: [PATCH v2026-07-08 1/2] injection_points: drive wait points through the filesystem, without SQL Add a filesystem-based way to attach a wait point and release a specific waiter, for code paths that run before the server can answer SQL (e.g. early postmaster startup). It works alongside the SQL path and does not touch the core registry. State lives under pg_injection_points/ in the data directory: pg_injection_points/<point>/ present -> <point> attached as a wait pg_injection_points/<point>/<pid> present -> that backend is parked here Each subdirectory is scanned once at startup and attached, so shared_preload_libraries is needed to arm a point before startup. A backend reaching the point publishes its <pid> file and polls with stat() until it is removed, besides watching the wakeup counter; removing the file wakes that one backend, and listing the directory shows which backends are blocked. The tree is dropped at shutdown. --- src/test/modules/injection_points/Makefile | 2 + .../injection_points/injection_points.c | 141 +++++++++++++++++- src/test/modules/injection_points/meson.build | 8 + .../t/001_wait_without_sql.pl | 86 +++++++++++ 4 files changed, 234 insertions(+), 3 deletions(-) create mode 100644 src/test/modules/injection_points/t/001_wait_without_sql.pl diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile index c01d2fb095c..3aca8c389de 100644 --- a/src/test/modules/injection_points/Makefile +++ b/src/test/modules/injection_points/Makefile @@ -24,6 +24,8 @@ ISOLATION = basic \ # some isolation tests require wal_level=replica ISOLATION_OPTS = --temp-config $(top_srcdir)/src/test/modules/injection_points/extra.conf +TAP_TESTS = 1 + # The injection points are cluster-wide, so disable installcheck NO_INSTALLCHECK = 1 diff --git a/src/test/modules/injection_points/injection_points.c b/src/test/modules/injection_points/injection_points.c index 2d26ecedd5d..893e9847402 100644 --- a/src/test/modules/injection_points/injection_points.c +++ b/src/test/modules/injection_points/injection_points.c @@ -17,6 +17,10 @@ #include "postgres.h" +#include <fcntl.h> +#include <sys/stat.h> + +#include "common/file_utils.h" #include "fmgr.h" #include "funcapi.h" #include "injection_points.h" @@ -24,6 +28,7 @@ #include "nodes/pg_list.h" #include "nodes/value.h" #include "storage/dsm_registry.h" +#include "storage/fd.h" #include "storage/ipc.h" #include "storage/lwlock.h" #include "storage/shmem.h" @@ -45,6 +50,39 @@ PG_MODULE_MAGIC; #define INJ_WAIT_INITIAL_US 10 /* 10us */ #define INJ_WAIT_MAX_US 100000 /* 100ms */ +/* + * Filesystem markers used to drive wait points without any SQL connection, for + * code paths that run before the server can answer SQL (e.g. early postmaster + * startup). They live under a directory in the data directory, one + * subdirectory per attached point and one file per parked process: + * + * pg_injection_points/<point>/ present = <point> attached as a wait; + * pg_injection_points/<point>/<pid> present = that process is parked here. + * + * A test attaches a point by creating its directory (scanned at startup), sees + * who is waiting by listing the directory, and wakes a specific waiter by + * removing its PID file. Everything is plain filesystem state checked with + * stat(), so there is no platform-specific logic and no out-of-process access + * to shared memory. Paths are relative to the data directory, which is the + * working directory of the postmaster and every backend. + */ +#define INJ_POINTS_DIR "pg_injection_points" + +/* "pg_injection_points/<point>" */ +static void +injection_point_dir(char *buf, size_t bufsize, const char *name) +{ + snprintf(buf, bufsize, "%s/%s", INJ_POINTS_DIR, name); +} + +/* "pg_injection_points/<point>/<pid>" */ +static void +injection_point_waiter_path(char *buf, size_t bufsize, + const char *name, int pid) +{ + snprintf(buf, bufsize, "%s/%s/%d", INJ_POINTS_DIR, name, pid); +} + /* * List of injection points stored in TopMemoryContext attached * locally to this process. @@ -117,6 +155,60 @@ injection_shmem_request(void *arg) ); } +/* + * Scan INJ_POINTS_DIR for point subdirectories and attach each as a wait point. + * Run once at postmaster startup so the points are in place before any process + * - the postmaster's own startup sequence or its children - reaches them, all + * without an SQL connection. A point can therefore be attached, by creating + * its directory, before the server is started. + */ +static void +injection_points_preload(void) +{ + DIR *dir; + struct dirent *de; + + dir = AllocateDir(INJ_POINTS_DIR); + if (dir == NULL) + return; /* no directory means nothing attached */ + + while ((de = ReadDir(dir, INJ_POINTS_DIR)) != NULL) + { + InjectionPointCondition condition = {.type = INJ_CONDITION_ALWAYS}; + char path[MAXPGPATH]; + struct stat st; + + if (strcmp(de->d_name, ".") == 0 || strcmp(de->d_name, "..") == 0) + continue; + if (strlen(de->d_name) >= INJ_NAME_MAXLEN) + continue; + + /* Each subdirectory names a wait point to attach. */ + injection_point_dir(path, sizeof(path), de->d_name); + if (stat(path, &st) != 0 || !S_ISDIR(st.st_mode)) + continue; + + InjectionPointAttach(de->d_name, "injection_points", "injection_wait", + &condition, sizeof(condition)); + } + FreeDir(dir); +} + +/* + * proc_exit callback that removes INJ_POINTS_DIR so markers do not survive the + * cluster. Forked children inherit this callback but must not run it, hence + * the IsUnderPostmaster guard: only the lifecycle owner (postmaster, or a + * standalone backend) cleans up. + */ +static void +injection_points_dir_cleanup(int code, Datum arg) +{ + struct stat st; + + if (!IsUnderPostmaster && stat(INJ_POINTS_DIR, &st) == 0) + (void) rmtree(INJ_POINTS_DIR, true); +} + static void injection_shmem_init(void *arg) { @@ -125,6 +217,13 @@ injection_shmem_init(void *arg) * initialization using a DSM. */ injection_point_init_state(inj_state, NULL); + + /* + * Attach any wait points requested out of band through marker directories, + * and make sure the directory does not outlive the cluster. + */ + injection_points_preload(); + on_proc_exit(injection_points_dir_cleanup, 0); } /* @@ -223,7 +322,7 @@ injection_notice(const char *name, const void *private_data, void *arg) elog(NOTICE, "notice triggered for injection point %s", name); } -/* Wait until injection_points_wakeup() is called */ +/* Wait until released by injection_points_wakeup() or a removed marker file */ void injection_wait(const char *name, const void *private_data, void *arg) { @@ -232,6 +331,9 @@ injection_wait(const char *name, const void *private_data, void *arg) uint32 injection_wait_event = 0; const InjectionPointCondition *condition = private_data; int delay_us = 0; + char waiter_path[MAXPGPATH]; + bool have_waiter_file = false; + struct stat st; if (inj_state == NULL) injection_init_shmem(); @@ -267,7 +369,35 @@ injection_wait(const char *name, const void *private_data, void *arg) name); /* - * Wait until the counter is bumped by injection_points_wakeup(). + * Publish our presence in the filesystem so an out-of-process test can see + * that we are parked here and release us with no SQL connection: create + * "pg_injection_points/<name>/<pid>" and wait until it is removed. The + * directories are created on demand, so this works for points attached + * through SQL too, not only those attached from a marker directory. + */ + { + char dirpath[MAXPGPATH]; + int fd; + + injection_point_dir(dirpath, sizeof(dirpath), name); + (void) MakePGDirectory(INJ_POINTS_DIR); + (void) MakePGDirectory(dirpath); + + injection_point_waiter_path(waiter_path, sizeof(waiter_path), name, + MyProcPid); + fd = OpenTransientFile(waiter_path, O_RDWR | O_CREAT | O_TRUNC); + if (fd >= 0) + { + CloseTransientFile(fd); + have_waiter_file = true; + } + } + + /* + * Wait until released, either by injection_points_wakeup() bumping the + * counter or by our waiter file being removed. The latter needs no SQL + * connection, so it works in code paths that run before the server can + * answer queries. * * This loop starts with a short delay for responsiveness, enlarged to * ease the CPU workload in slower environments. @@ -275,7 +405,8 @@ injection_wait(const char *name, const void *private_data, void *arg) delay_us = INJ_WAIT_INITIAL_US; pgstat_report_wait_start(injection_wait_event); - while (pg_atomic_read_u32(&inj_state->wait_counts[index]) == old_wait_counts) + while (pg_atomic_read_u32(&inj_state->wait_counts[index]) == old_wait_counts && + (!have_waiter_file || stat(waiter_path, &st) == 0)) { CHECK_FOR_INTERRUPTS(); pg_usleep(delay_us); @@ -284,6 +415,10 @@ injection_wait(const char *name, const void *private_data, void *arg) } pgstat_report_wait_end(); + /* Clean up our waiter file; it may still exist after a counter wakeup. */ + if (have_waiter_file) + (void) unlink(waiter_path); + /* Remove this injection point from the waiters. */ SpinLockAcquire(&inj_state->lock); inj_state->name[index][0] = '\0'; diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build index 59dba1cb023..f6b53e1a33f 100644 --- a/src/test/modules/injection_points/meson.build +++ b/src/test/modules/injection_points/meson.build @@ -60,4 +60,12 @@ tests += { '--temp-config', files('extra.conf'), ], }, + 'tap': { + 'env': { + 'enable_injection_points': get_option('injection_points') ? 'yes' : 'no', + }, + 'tests': [ + 't/001_wait_without_sql.pl', + ], + }, } diff --git a/src/test/modules/injection_points/t/001_wait_without_sql.pl b/src/test/modules/injection_points/t/001_wait_without_sql.pl new file mode 100644 index 00000000000..8c33e99f24f --- /dev/null +++ b/src/test/modules/injection_points/t/001_wait_without_sql.pl @@ -0,0 +1,86 @@ +# Copyright (c) 2026, PostgreSQL Global Development Group + +# Exercise driving a wait injection point purely through the filesystem, with +# no SQL used for the coordination itself. This is meant for code paths that +# run before the server can answer queries (e.g. early postmaster startup). +# +# Layout under the data directory: +# pg_injection_points/<point>/ present -> <point> attached as a wait +# pg_injection_points/<point>/<pid> present -> that backend is parked here +# Removing the <pid> file wakes that backend. + +use strict; +use warnings FATAL => 'all'; + +use Time::HiRes qw(usleep); + +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; +use Test::More; + +if ($ENV{enable_injection_points} ne 'yes') +{ + plan skip_all => 'Injection points not supported by this build'; +} + +my $point = 'no-sql-wait'; + +my $node = PostgreSQL::Test::Cluster->new('node'); +$node->init; +$node->append_conf('postgresql.conf', + "shared_preload_libraries = 'injection_points'"); + +# Attach the wait point before the server is even running, without any SQL: +# just create its directory. The module scans these at startup. +my $root = $node->data_dir . '/pg_injection_points'; +my $pdir = "$root/$point"; +mkdir $root or die "could not create $root: $!"; +mkdir $pdir or die "could not create $pdir: $!"; + +$node->start; + +# The module attached the point from its directory at startup; we never called +# injection_points_attach(). +$node->safe_psql('postgres', 'CREATE EXTENSION injection_points;'); + +my $listed = $node->safe_psql('postgres', + "SELECT count(*) FROM injection_points_list() WHERE point_name = '$point';" +); +is($listed, '1', 'wait point attached from its directory, without SQL'); + +# Fire the point in a background session; it must block in injection_wait(). +my $bg = $node->background_psql('postgres'); +$bg->query_until( + qr/start/, qq[ +\\echo start +SELECT injection_points_run('$point'); +]); + +# Detect that the backend is parked, purely through the filesystem: it +# publishes a file named after its PID inside the point directory. +my $waiter; +foreach my $i (1 .. 1800) +{ + if (opendir(my $dh, $pdir)) + { + ($waiter) = grep { /^\d+\z/ } readdir($dh); + closedir($dh); + last if defined $waiter; + } + usleep(100_000); +} +ok(defined $waiter, 'backend published its waiter file (filesystem-observable)'); + +# Wake that specific backend without any SQL: remove its waiter file. +unlink "$pdir/$waiter" or die "could not remove waiter file: $!"; + +# The blocked statement now finishes, proving the filesystem wakeup worked. +like($bg->query_safe('SELECT 1;'), + qr/^1$/m, 'backend released by removing its waiter file, without SQL'); +$bg->quit; + +# The directory does not survive the cluster. +$node->stop; +ok(!-d $root, 'injection points directory removed at shutdown'); + +done_testing(); -- 2.50.1 (Apple Git-155) [application/octet-stream] v2026-07-08-0002-injection_points-test-that-pre-auth-back.patch (6.8K, ../../[email protected]/3-v2026-07-08-0002-injection_points-test-that-pre-auth-back.patch) download | inline diff: From 5a19fe80da260375353037dd44223a024dd66bd0 Mon Sep 17 00:00:00 2001 From: Andrey Borodin <[email protected]> Date: Wed, 8 Jul 2026 21:17:42 +0500 Subject: [PATCH v2026-07-08 2/2] injection_points: test that pre-auth backends hold connection slots A backend claims its PGPROC in InitProcessPhase2(), before authentication, so max_connections must bound in-flight connections, not just authenticated sessions. Add a TAP test that fills every slot with connections frozen before authentication, checks that one more is refused with "too many clients", and that releasing one frees the slot again. Such a backend cannot be driven or identified over SQL or pg_stat_activity, so the test holds, observes and releases it through the init-pre-auth wait point and the module's filesystem markers. --- src/test/modules/injection_points/meson.build | 1 + .../injection_points/t/002_preauth_slots.pl | 147 ++++++++++++++++++ 2 files changed, 148 insertions(+) create mode 100644 src/test/modules/injection_points/t/002_preauth_slots.pl diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build index f6b53e1a33f..6074b373393 100644 --- a/src/test/modules/injection_points/meson.build +++ b/src/test/modules/injection_points/meson.build @@ -66,6 +66,7 @@ tests += { }, 'tests': [ 't/001_wait_without_sql.pl', + 't/002_preauth_slots.pl', ], }, } diff --git a/src/test/modules/injection_points/t/002_preauth_slots.pl b/src/test/modules/injection_points/t/002_preauth_slots.pl new file mode 100644 index 00000000000..5c5d46bef68 --- /dev/null +++ b/src/test/modules/injection_points/t/002_preauth_slots.pl @@ -0,0 +1,147 @@ +# Copyright (c) 2026, PostgreSQL Global Development Group + +# Check that a connection which has not authenticated yet already counts +# against max_connections. +# +# A backend claims its PGPROC entry in InitProcessPhase2(), and "sorry, too +# many clients already" is reported at slot acquisition time -- both happen +# before authentication. The invariant under test is therefore that +# max_connections bounds all in-flight connections, not just authenticated +# sessions. If that ever regressed (e.g. slots were accounted only after +# authentication), unauthenticated clients could overrun the limit, and the +# reserved_connections logic, which counts free PGPROCs including the pre-auth +# ones, would hand out reserved slots it should not. +# +# Testing this needs a connection frozen in the narrow window after it has +# taken a slot but before it authenticates, and such a backend cannot be +# driven or even identified over SQL or pg_stat_activity. The injection_points +# module's "init-pre-auth" wait point and its filesystem markers are just the +# tool used to hold a backend there and to observe/release it out of process. + +use strict; +use warnings FATAL => 'all'; + +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; +use Test::More; +use Time::HiRes qw(usleep); + +if ($ENV{enable_injection_points} ne 'yes') +{ + plan skip_all => 'Injection points not supported by this build'; +} + +# Small, fully deterministic connection budget. Client backends draw from the +# regular PGPROC freelist sized by max_connections; autovacuum, background +# workers and walsenders use separate pools, so this arithmetic is exact. +my $max_conn = 4; + +my $node = PostgreSQL::Test::Cluster->new('main'); +$node->init; +$node->append_conf( + 'postgresql.conf', qq[ +autovacuum = off +max_connections = $max_conn +superuser_reserved_connections = 0 +reserved_connections = 0 +]); +$node->start; + +$node->safe_psql('postgres', 'CREATE EXTENSION injection_points;'); + +my $inj_dir = $node->data_dir . '/pg_injection_points/init-pre-auth'; +my $timeout_us = $PostgreSQL::Test::Utils::timeout_default * 1_000_000; + +# Return the set of pids currently parked at init-pre-auth, as a hash ref. +sub parked_pids +{ + my %pids; + if (opendir(my $dh, $inj_dir)) + { + %pids = map { $_ => 1 } grep { /^\d+$/ } readdir $dh; + closedir $dh; + } + return \%pids; +} + +# Wait until a pid that is not in %$before shows up, and return it. +sub wait_for_new_parked_pid +{ + my ($before) = @_; + my $waited = 0; + while (1) + { + my $now = parked_pids(); + for my $pid (keys %$now) + { + return $pid unless $before->{$pid}; + } + die "timed out waiting for a backend to park at init-pre-auth" + if $waited > $timeout_us; + usleep(10_000); + $waited += 10_000; + } +} + +# The controller session stays connected for the whole armed window; it arms +# the wait point and never exits while it is attached, so it never parks. +my $ctl = $node->background_psql('postgres'); +$ctl->query_safe("SELECT injection_points_attach('init-pre-auth', 'wait')"); + +# Open victims that will hang before authentication, each consuming one slot. +# We do not (indeed cannot) authenticate them; we only learn their pids from +# the marker directory. With the controller holding one slot, max_connections +# - 1 victims exactly fill the remaining slots. +my @victims; +my @victim_pids; +for (my $i = 0; $i < $max_conn - 1; $i++) +{ + my $before = parked_pids(); + my $v = $node->background_psql('postgres', wait => 0); + push @victims, $v; + push @victim_pids, wait_for_new_parked_pid($before); +} + +is(scalar(@victim_pids), $max_conn - 1, + 'all pre-auth victims are parked and visible in the filesystem'); + +# Every slot is now taken by pre-authentication backends plus the controller. +# A fresh connection is refused before it ever reaches the wait point, which +# proves the parked backends really do hold connection slots. +$node->connect_fails( + 'dbname=postgres', + 'pre-auth backends occupy connection slots', + expected_stderr => qr/FATAL: sorry, too many clients already/); + +# Detach the point so that released backends, and the probe connection below, +# do not park again. Backends already parked keep waiting on their own marker +# files, so they keep holding their slots until we remove those files. +$ctl->query_safe("SELECT injection_points_detach('init-pre-auth')"); + +# Let one pre-auth backend proceed (unlink its marker) and disconnect. If the +# slot it was holding while unauthenticated is now reusable, the connection +# limit was accounting that pre-auth backend and nothing else. +my $freed_pid = shift @victim_pids; +my $freed_victim = shift @victims; +unlink "$inj_dir/$freed_pid" or die "unlink $inj_dir/$freed_pid: $!"; +$freed_victim->wait_connect; +$freed_victim->quit; + +is($node->safe_psql('postgres', 'SELECT 1'), + '1', 'freed pre-auth slot becomes available for a new connection'); + +# Release and reap the remaining victims, then shut everything down. +while (@victims) +{ + my $pid = shift @victim_pids; + my $v = shift @victims; + unlink "$inj_dir/$pid"; + eval { + $v->wait_connect; + $v->quit; + }; +} +$ctl->quit; +$node->stop; + +done_testing(); -- 2.50.1 (Apple Git-155) ^ permalink raw reply [nested|flat] 21+ messages in thread
end of thread, other threads:[~2026-07-08 19:17 UTC | newest] Thread overview: 21+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2026-05-28 02:43 injection_points: Switch wait/wakeup to use atomics rather than latches Michael Paquier <[email protected]> 2026-05-28 12:40 ` Robert Haas <[email protected]> 2026-05-28 23:19 ` Michael Paquier <[email protected]> 2026-05-29 12:48 ` Robert Haas <[email protected]> 2026-05-29 13:31 ` Heikki Linnakangas <[email protected]> 2026-05-29 16:00 ` Robert Haas <[email protected]> 2026-05-30 04:13 ` Michael Paquier <[email protected]> 2026-05-30 08:05 ` Andrey Borodin <[email protected]> 2026-06-01 11:25 ` Andrey Borodin <[email protected]> 2026-06-02 04:15 ` Michael Paquier <[email protected]> 2026-06-02 05:13 ` Andrey Borodin <[email protected]> 2026-06-02 05:27 ` Michael Paquier <[email protected]> 2026-06-02 18:46 ` Andrey Borodin <[email protected]> 2026-06-02 22:23 ` Michael Paquier <[email protected]> 2026-06-12 07:02 ` Andrey Borodin <[email protected]> 2026-06-14 09:23 ` Andrey Borodin <[email protected]> 2026-06-15 00:14 ` Michael Paquier <[email protected]> 2026-07-07 09:51 ` Andrey Borodin <[email protected]> 2026-07-08 06:29 ` Michael Paquier <[email protected]> 2026-07-08 07:17 ` Heikki Linnakangas <[email protected]> 2026-07-08 19:17 ` Andrey Borodin <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox