public inbox for [email protected]
help / color / mirror / Atom feedFrom: Joel Jacobson <[email protected]>
To: pgsql-hackers <[email protected]>
Subject: [PATCH] Fix LISTEN startup race with direct advancement
Date: Tue, 19 May 2026 13:37:56 -0700
Message-ID: <[email protected]> (raw)
Hi hackers,
I had another pass over the async.c rework committed in 282b1cd,
and found a race that can cause a notification committed after
the listener registered its queue position to be missed entirely.
This can happen in the small time window between PreCommit_Notify(),
where the first LISTEN registers the backend and records its queue
position, and AtCommit_Notify(), where the staged listen action is
made active in the shared channel map by setting listening = true.
If a concurrent NOTIFY commits in that window, SignalBackends() can
see the staged listener entry with listening = false and conclude that
the backend is not interested in the channel. With direct advancement,
that can move the backend's queue pointer past the notification instead
of waking it.
This is distinct from the documented LISTEN startup race in
listen.sgml. The documented race can produce false positives: after
LISTEN returns, an application may receive a notification for work
already observed by its initial database scan. That is harmless. This
race is a false negative: a notification can be missed entirely.
The fix is just to treat staged LISTEN entries as possible listeners
when deciding whom to wake:
```diff
- if (!listeners[j].listening)
- continue; /* ignore not-yet-committed listeners */
```
The attached patches split the report into tests and fix:
0001 Test missed LISTEN startup notification
0002 Test LISTEN startup notification for already-seen work
0003 Fix LISTEN startup race with direct advancement
/Joel
Attachments:
[application/octet-stream] 0001-Test-missed-LISTEN-startup-notification.patch (5.4K, 2-0001-Test-missed-LISTEN-startup-notification.patch)
download | inline diff:
From 16c94f3d118a4da02b37b9a45d2abafda6b984f4 Mon Sep 17 00:00:00 2001
From: Joel Jacobson <[email protected]>
Date: Tue, 19 May 2026 10:44:46 -0700
Subject: [PATCH 1/3] Test missed LISTEN startup notification
Add an injection-point isolation test that pauses a first LISTEN before
AtCommit_Notify() applies the pending listen action. A concurrent
NOTIFY in that window should be delivered when LISTEN returns.
This is distinct from the setup race documented in listen.sgml. That
race can produce false positives: a new listener can receive
notifications for changes it already saw during its initial database
inspection. Such extra notifications are harmless.
The race tested here is a false negative. The listener has already
registered its queue position, and its channel table entry is visible
to SignalBackends(). However, AtCommit_Notify() has not yet marked it
listening=true, so a notification committed after that point can be
skipped and lost entirely.
Before the fix, this test fails by missing that notification. The test
would have passed before the LISTEN/NOTIFY rework in 282b1cd, which
introduced the shared channel map and direct advancement.
---
src/backend/commands/async.c | 4 ++
src/test/modules/injection_points/Makefile | 1 +
.../expected/async-notify-listen-startup.out | 18 +++++++++
src/test/modules/injection_points/meson.build | 1 +
.../specs/async-notify-listen-startup.spec | 38 +++++++++++++++++++
5 files changed, 62 insertions(+)
create mode 100644 src/test/modules/injection_points/expected/async-notify-listen-startup.out
create mode 100644 src/test/modules/injection_points/specs/async-notify-listen-startup.spec
diff --git a/src/backend/commands/async.c b/src/backend/commands/async.c
index db6a9a6561b..cefd5297a73 100644
--- a/src/backend/commands/async.c
+++ b/src/backend/commands/async.c
@@ -184,6 +184,7 @@
#include "utils/builtins.h"
#include "utils/dsa.h"
#include "utils/guc_hooks.h"
+#include "utils/injection_point.h"
#include "utils/memutils.h"
#include "utils/ps_status.h"
#include "utils/snapmgr.h"
@@ -1387,6 +1388,9 @@ AtCommit_Notify(void)
if (Trace_notify)
elog(DEBUG1, "AtCommit_Notify");
+ if (pendingActions != NULL)
+ INJECTION_POINT("async-notify-before-listen-commit", NULL);
+
/* Apply staged listen/unlisten changes */
ApplyPendingListenActions(true);
diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile
index c01d2fb095c..37c1b1cffb6 100644
--- a/src/test/modules/injection_points/Makefile
+++ b/src/test/modules/injection_points/Makefile
@@ -13,6 +13,7 @@ REGRESS = injection_points hashagg reindex_conc vacuum
REGRESS_OPTS = --dlpath=$(top_builddir)/src/test/regress
ISOLATION = basic \
+ async-notify-listen-startup \
inplace \
repack \
repack_temporal \
diff --git a/src/test/modules/injection_points/expected/async-notify-listen-startup.out b/src/test/modules/injection_points/expected/async-notify-listen-startup.out
new file mode 100644
index 00000000000..d65e5015cff
--- /dev/null
+++ b/src/test/modules/injection_points/expected/async-notify-listen-startup.out
@@ -0,0 +1,18 @@
+Parsed test spec with 3 sessions
+
+starting permutation: listen notify wake check
+step listen: LISTEN race; <waiting ...>
+step notify: NOTIFY race, 'payload';
+step wake:
+ SELECT FROM injection_points_detach('async-notify-before-listen-commit');
+ SELECT FROM injection_points_wakeup('async-notify-before-listen-commit');
+ <waiting ...>
+step listen: <... completed>
+listener: NOTIFY "race" with payload "payload" from notifier
+step check: SELECT 1 AS x;
+x
+-
+1
+(1 row)
+
+step wake: <... completed>
diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build
index 59dba1cb023..61a68bcfe15 100644
--- a/src/test/modules/injection_points/meson.build
+++ b/src/test/modules/injection_points/meson.build
@@ -44,6 +44,7 @@ tests += {
'isolation': {
'specs': [
'basic',
+ 'async-notify-listen-startup',
'inplace',
'repack',
'repack_temporal',
diff --git a/src/test/modules/injection_points/specs/async-notify-listen-startup.spec b/src/test/modules/injection_points/specs/async-notify-listen-startup.spec
new file mode 100644
index 00000000000..29832a514d0
--- /dev/null
+++ b/src/test/modules/injection_points/specs/async-notify-listen-startup.spec
@@ -0,0 +1,38 @@
+# Test LISTEN/NOTIFY startup behavior while committing the first LISTEN.
+#
+# A first LISTEN registers the backend's queue position before the transaction
+# becomes visible, then commits the local listen state later in AtCommit_Notify.
+# A concurrent NOTIFY in that window must still wake the listener, so the
+# notification is delivered after LISTEN returns.
+
+setup
+{
+ CREATE EXTENSION injection_points;
+}
+
+teardown
+{
+ DROP EXTENSION injection_points;
+}
+
+session listener
+setup
+{
+ SELECT FROM injection_points_set_local();
+ SELECT FROM injection_points_attach('async-notify-before-listen-commit', 'wait');
+}
+step listen { LISTEN race; }
+step check { SELECT 1 AS x; }
+teardown { UNLISTEN *; }
+
+session notifier
+step notify { NOTIFY race, 'payload'; }
+
+session controller
+step wake
+{
+ SELECT FROM injection_points_detach('async-notify-before-listen-commit');
+ SELECT FROM injection_points_wakeup('async-notify-before-listen-commit');
+}
+
+permutation listen notify wake(listen) check
--
2.52.0
[application/octet-stream] 0003-Fix-LISTEN-startup-race-with-direct-advancement.patch (6.5K, 3-0003-Fix-LISTEN-startup-race-with-direct-advancement.patch)
download | inline diff:
From ea0f7e81c710328446cf8dd51de63458619697dd Mon Sep 17 00:00:00 2001
From: Joel Jacobson <[email protected]>
Date: Tue, 19 May 2026 07:36:25 -0700
Subject: [PATCH 3/3] Fix LISTEN startup race with direct advancement
LISTEN creates its shared channel-map entry before commit, with
listening=false until AtCommit_Notify() applies the staged action. A
concurrent NOTIFY can see that commit in the window before the flag is
flipped. SignalBackends() treated that backend as not interested, so
direct advancement could move its queue pointer past the notification.
Do not test listeners[j].listening when deciding whom to wake. A
false value can mean a LISTEN that has registered its queue position,
but has not yet run AtCommit_Notify(). Waking such a backend is
harmless if it aborts; advancing it here could make it miss a
notification after commit.
The preceding missed-notification test now passes with the fix. Add a
matching regression test for the fixed behavior.
---
src/backend/commands/async.c | 21 ++++++----
src/test/modules/injection_points/Makefile | 1 +
.../expected/async-notify-listen-race.out | 18 +++++++++
src/test/modules/injection_points/meson.build | 1 +
.../specs/async-notify-listen-race.spec | 38 +++++++++++++++++++
5 files changed, 71 insertions(+), 8 deletions(-)
create mode 100644 src/test/modules/injection_points/expected/async-notify-listen-race.out
create mode 100644 src/test/modules/injection_points/specs/async-notify-listen-race.spec
diff --git a/src/backend/commands/async.c b/src/backend/commands/async.c
index bd7eff7f305..f1bd278bd19 100644
--- a/src/backend/commands/async.c
+++ b/src/backend/commands/async.c
@@ -114,11 +114,11 @@
* If the current transaction has executed any LISTEN/UNLISTEN actions,
* PreCommit_Notify() prepares to commit those. For LISTEN, it
* pre-allocates entries in both the per-backend localChannelTable and the
- * shared globalChannelTable (with listening=false so that these entries
- * are no-ops for the moment). It also records the final per-channel
- * intent in pendingListenActions, so post-commit/abort processing can
- * apply that in a single step. Since all these allocations happen before
- * committing to clog, we can safely abort the transaction on failure.
+ * shared globalChannelTable (with listening=false to mark these entries
+ * as staged). It also records the final per-channel intent in
+ * pendingListenActions, so post-commit/abort processing can apply that in
+ * a single step. Since all these allocations happen before committing to
+ * clog, we can safely abort the transaction on failure.
*
* After commit, AtCommit_Notify() runs through pendingListenActions and
* updates the backend's per-channel listening flags to activate or
@@ -2311,9 +2311,6 @@ SignalBackends(void)
int32 pid;
QueuePosition pos;
- if (!listeners[j].listening)
- continue; /* ignore not-yet-committed listeners */
-
i = listeners[j].procNo;
if (QUEUE_BACKEND_WAKEUP_PENDING(i))
@@ -2327,6 +2324,14 @@ SignalBackends(void)
Assert(pid != InvalidPid);
+ /*
+ * Do not test listeners[j].listening here. A false value can
+ * mean a LISTEN that has registered its queue position, but has
+ * not yet run AtCommit_Notify(). Waking such a backend is
+ * harmless if it aborts; advancing it here could make it miss a
+ * notification after commit.
+ */
+
QUEUE_BACKEND_WAKEUP_PENDING(i) = true;
signalPids[count] = pid;
signalProcnos[count] = i;
diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile
index dcf442df9ba..665e7852d52 100644
--- a/src/test/modules/injection_points/Makefile
+++ b/src/test/modules/injection_points/Makefile
@@ -15,6 +15,7 @@ REGRESS_OPTS = --dlpath=$(top_builddir)/src/test/regress
ISOLATION = basic \
async-notify-listen-false-positive \
async-notify-listen-startup \
+ async-notify-listen-race \
inplace \
repack \
repack_temporal \
diff --git a/src/test/modules/injection_points/expected/async-notify-listen-race.out b/src/test/modules/injection_points/expected/async-notify-listen-race.out
new file mode 100644
index 00000000000..d65e5015cff
--- /dev/null
+++ b/src/test/modules/injection_points/expected/async-notify-listen-race.out
@@ -0,0 +1,18 @@
+Parsed test spec with 3 sessions
+
+starting permutation: listen notify wake check
+step listen: LISTEN race; <waiting ...>
+step notify: NOTIFY race, 'payload';
+step wake:
+ SELECT FROM injection_points_detach('async-notify-before-listen-commit');
+ SELECT FROM injection_points_wakeup('async-notify-before-listen-commit');
+ <waiting ...>
+step listen: <... completed>
+listener: NOTIFY "race" with payload "payload" from notifier
+step check: SELECT 1 AS x;
+x
+-
+1
+(1 row)
+
+step wake: <... completed>
diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build
index 8a91b0a41aa..7fb8de3d0f8 100644
--- a/src/test/modules/injection_points/meson.build
+++ b/src/test/modules/injection_points/meson.build
@@ -46,6 +46,7 @@ tests += {
'basic',
'async-notify-listen-false-positive',
'async-notify-listen-startup',
+ 'async-notify-listen-race',
'inplace',
'repack',
'repack_temporal',
diff --git a/src/test/modules/injection_points/specs/async-notify-listen-race.spec b/src/test/modules/injection_points/specs/async-notify-listen-race.spec
new file mode 100644
index 00000000000..3dd6bcfa8b0
--- /dev/null
+++ b/src/test/modules/injection_points/specs/async-notify-listen-race.spec
@@ -0,0 +1,38 @@
+# Test LISTEN/NOTIFY race around the LISTEN transaction's commit.
+#
+# A LISTEN backend registers its queue position before commit, but only applies
+# local listen state in AtCommit_Notify(). A concurrent NOTIFY that commits
+# while the LISTEN backend is between those two points must not advance the
+# listener past the notification.
+
+setup
+{
+ CREATE EXTENSION injection_points;
+}
+
+teardown
+{
+ DROP EXTENSION injection_points;
+}
+
+session listener
+setup
+{
+ SELECT FROM injection_points_set_local();
+ SELECT FROM injection_points_attach('async-notify-before-listen-commit', 'wait');
+}
+step listen { LISTEN race; }
+step check { SELECT 1 AS x; }
+teardown { UNLISTEN *; }
+
+session notifier
+step notify { NOTIFY race, 'payload'; }
+
+session controller
+step wake
+{
+ SELECT FROM injection_points_detach('async-notify-before-listen-commit');
+ SELECT FROM injection_points_wakeup('async-notify-before-listen-commit');
+}
+
+permutation listen notify wake(listen) check
--
2.52.0
[application/octet-stream] 0002-Test-LISTEN-startup-notification-for-already-seen-wo.patch (5.1K, 4-0002-Test-LISTEN-startup-notification-for-already-seen-wo.patch)
download | inline diff:
From 8fcba03aabc2defbad75628b337fa0c651039a50 Mon Sep 17 00:00:00 2001
From: Joel Jacobson <[email protected]>
Date: Tue, 19 May 2026 10:55:50 -0700
Subject: [PATCH 2/3] Test LISTEN startup notification for already-seen work
Add an injection-point isolation test that pauses a first LISTEN after
the pending listen action has been applied, but before the command
returns to the client. A concurrent transaction inserts a row and
sends a NOTIFY while the listener is paused.
When LISTEN returns, the listener receives one notification and its
initial table scan sees the same row. This demonstrates the harmless
false positive described by the LISTEN documentation: applications may
observe the same work through both the startup scan and the
notification stream, and should tolerate that.
---
src/backend/commands/async.c | 3 ++
src/test/modules/injection_points/Makefile | 1 +
.../async-notify-listen-false-positive.out | 21 +++++++++
src/test/modules/injection_points/meson.build | 1 +
.../async-notify-listen-false-positive.spec | 43 +++++++++++++++++++
5 files changed, 69 insertions(+)
create mode 100644 src/test/modules/injection_points/expected/async-notify-listen-false-positive.out
create mode 100644 src/test/modules/injection_points/specs/async-notify-listen-false-positive.spec
diff --git a/src/backend/commands/async.c b/src/backend/commands/async.c
index cefd5297a73..bd7eff7f305 100644
--- a/src/backend/commands/async.c
+++ b/src/backend/commands/async.c
@@ -1394,6 +1394,9 @@ AtCommit_Notify(void)
/* Apply staged listen/unlisten changes */
ApplyPendingListenActions(true);
+ if (pendingActions != NULL)
+ INJECTION_POINT("async-notify-after-listen-commit", NULL);
+
/* If no longer listening to anything, get out of listener array */
if (amRegisteredListener && LocalChannelTableIsEmpty())
asyncQueueUnregister();
diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile
index 37c1b1cffb6..dcf442df9ba 100644
--- a/src/test/modules/injection_points/Makefile
+++ b/src/test/modules/injection_points/Makefile
@@ -13,6 +13,7 @@ REGRESS = injection_points hashagg reindex_conc vacuum
REGRESS_OPTS = --dlpath=$(top_builddir)/src/test/regress
ISOLATION = basic \
+ async-notify-listen-false-positive \
async-notify-listen-startup \
inplace \
repack \
diff --git a/src/test/modules/injection_points/expected/async-notify-listen-false-positive.out b/src/test/modules/injection_points/expected/async-notify-listen-false-positive.out
new file mode 100644
index 00000000000..4cab951ca55
--- /dev/null
+++ b/src/test/modules/injection_points/expected/async-notify-listen-false-positive.out
@@ -0,0 +1,21 @@
+Parsed test spec with 3 sessions
+
+starting permutation: listen notify wake inspect
+step listen: LISTEN race; <waiting ...>
+step notify:
+ INSERT INTO notify_queue VALUES (1);
+ NOTIFY race, '1';
+
+step wake:
+ SELECT FROM injection_points_detach('async-notify-after-listen-commit');
+ SELECT FROM injection_points_wakeup('async-notify-after-listen-commit');
+ <waiting ...>
+step listen: <... completed>
+listener: NOTIFY "race" with payload "1" from notifier
+step inspect: SELECT id FROM notify_queue ORDER BY id;
+id
+--
+ 1
+(1 row)
+
+step wake: <... completed>
diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build
index 61a68bcfe15..8a91b0a41aa 100644
--- a/src/test/modules/injection_points/meson.build
+++ b/src/test/modules/injection_points/meson.build
@@ -44,6 +44,7 @@ tests += {
'isolation': {
'specs': [
'basic',
+ 'async-notify-listen-false-positive',
'async-notify-listen-startup',
'inplace',
'repack',
diff --git a/src/test/modules/injection_points/specs/async-notify-listen-false-positive.spec b/src/test/modules/injection_points/specs/async-notify-listen-false-positive.spec
new file mode 100644
index 00000000000..ad31c37acb6
--- /dev/null
+++ b/src/test/modules/injection_points/specs/async-notify-listen-false-positive.spec
@@ -0,0 +1,43 @@
+# Test harmless duplicate notification during LISTEN startup.
+#
+# A notification can arrive for work that the first database inspection after
+# LISTEN already observes. Applications should inspect database state after
+# LISTEN returns and tolerate a few notifications for rows already seen.
+
+setup
+{
+ CREATE EXTENSION injection_points;
+ CREATE TABLE notify_queue(id int primary key);
+}
+
+teardown
+{
+ DROP TABLE notify_queue;
+ DROP EXTENSION injection_points;
+}
+
+session listener
+setup
+{
+ SELECT FROM injection_points_set_local();
+ SELECT FROM injection_points_attach('async-notify-after-listen-commit', 'wait');
+}
+step listen { LISTEN race; }
+step inspect { SELECT id FROM notify_queue ORDER BY id; }
+teardown { UNLISTEN *; }
+
+session notifier
+step notify
+{
+ INSERT INTO notify_queue VALUES (1);
+ NOTIFY race, '1';
+}
+
+session controller
+step wake
+{
+ SELECT FROM injection_points_detach('async-notify-after-listen-commit');
+ SELECT FROM injection_points_wakeup('async-notify-after-listen-commit');
+}
+
+permutation listen notify wake(listen) inspect
--
2.52.0
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]
Subject: Re: [PATCH] Fix LISTEN startup race with direct advancement
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