public inbox for [email protected]
help / color / mirror / Atom feedFrom: Andrey Borodin <[email protected]>
To: Michael Paquier <[email protected]>
Cc: Robert Haas <[email protected]>
Cc: Postgres hackers <[email protected]>
Cc: Heikki Linnakangas <[email protected]>
Subject: Re: injection_points: Switch wait/wakeup to use atomics rather than latches
Date: Tue, 7 Jul 2026 14:51:04 +0500
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[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)
view thread (21+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: injection_points: Switch wait/wakeup to use atomics rather than latches
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox