public inbox for [email protected]help / color / mirror / Atom feed
[PATCH] Fix LISTEN startup race with direct advancement 8+ messages / 3 participants [nested] [flat]
* [PATCH] Fix LISTEN startup race with direct advancement @ 2026-05-19 20:37 Joel Jacobson <[email protected]> 2026-05-20 11:01 ` Re: [PATCH] Fix LISTEN startup race with direct advancement Arseniy Mukhin <[email protected]> 0 siblings, 1 reply; 8+ messages in thread From: Joel Jacobson @ 2026-05-19 20:37 UTC (permalink / raw) To: pgsql-hackers 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 ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: [PATCH] Fix LISTEN startup race with direct advancement 2026-05-19 20:37 [PATCH] Fix LISTEN startup race with direct advancement Joel Jacobson <[email protected]> @ 2026-05-20 11:01 ` Arseniy Mukhin <[email protected]> 2026-05-20 11:20 ` Re: [PATCH] Fix LISTEN startup race with direct advancement Joel Jacobson <[email protected]> 0 siblings, 1 reply; 8+ messages in thread From: Arseniy Mukhin @ 2026-05-20 11:01 UTC (permalink / raw) To: Joel Jacobson <[email protected]>; +Cc: pgsql-hackers On Tue, May 19, 2026 at 11:39 PM Joel Jacobson <[email protected]> wrote: > > 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 Hi, Thank you for the fix! I tried the test and managed to reproduce it. I think what is definitely wrong here is that we can do direct advancement over messages that the listener is interested in, when it has set the position already. So +1 for the fix. I think the fix is correct and it's IIUC really harmless as the worst thing that can happen is an unnecessary signal. One point - looks like the 0003 contains the same test as 0001. Also while I was trying to understand the issue, another bad schedule came to my mind: it's basically the same scenario as you reproduced, but now we have a message in between the listener's position and notification that the listener is interested in. In this case the notifier can't do direct advancement, so the message is not lost, but the notifier still doesn't signal about a new message, so the listener doesn't know that there is something to read until something triggers it (like another message). PFA the reproducer (there is a schedule diagram in the head of the file). It depends on 0001. Your patch fixes it too. Thank you! Best regards, Arseniy Mukhin Attachments: [application/octet-stream] untriggered-notification.patch.nocfbot (6.5K, 2-untriggered-notification.patch.nocfbot) download ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: [PATCH] Fix LISTEN startup race with direct advancement 2026-05-19 20:37 [PATCH] Fix LISTEN startup race with direct advancement Joel Jacobson <[email protected]> 2026-05-20 11:01 ` Re: [PATCH] Fix LISTEN startup race with direct advancement Arseniy Mukhin <[email protected]> @ 2026-05-20 11:20 ` Joel Jacobson <[email protected]> 2026-05-20 17:27 ` Re: [PATCH] Fix LISTEN startup race with direct advancement Arseniy Mukhin <[email protected]> 0 siblings, 1 reply; 8+ messages in thread From: Joel Jacobson @ 2026-05-20 11:20 UTC (permalink / raw) To: Arseniy Mukhin <[email protected]>; +Cc: pgsql-hackers On Wed, May 20, 2026, at 04:01, Arseniy Mukhin wrote: > One point - looks like the 0003 contains the same test as 0001. They are similar, but if you look carefully you'll see that they use different injection points: 0001 uses async-notify-before-listen-commit 0002 uses async-notify-after-listen-commit Another different is that if you only apply 0001 and run the tests, that test will fail without the 0003 fix, whereas 0002 pass both with and without 0003, since it just tests and demonstrates the already documented quite harmless false positive race condition. > Also while I was trying to understand the issue ... > diagram in the head of the file). It depends on 0001. Your patch fixes > it too. OK, I didn't look in detail at untriggered-notification.patch.nocfbot, but good to hear 0003 fixes it too. Thanks for reviewing. /Joel ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: [PATCH] Fix LISTEN startup race with direct advancement 2026-05-19 20:37 [PATCH] Fix LISTEN startup race with direct advancement Joel Jacobson <[email protected]> 2026-05-20 11:01 ` Re: [PATCH] Fix LISTEN startup race with direct advancement Arseniy Mukhin <[email protected]> 2026-05-20 11:20 ` Re: [PATCH] Fix LISTEN startup race with direct advancement Joel Jacobson <[email protected]> @ 2026-05-20 17:27 ` Arseniy Mukhin <[email protected]> 2026-05-21 04:49 ` Re: [PATCH] Fix LISTEN startup race with direct advancement Joel Jacobson <[email protected]> 0 siblings, 1 reply; 8+ messages in thread From: Arseniy Mukhin @ 2026-05-20 17:27 UTC (permalink / raw) To: Joel Jacobson <[email protected]>; +Cc: pgsql-hackers On Wed, May 20, 2026 at 2:20 PM Joel Jacobson <[email protected]> wrote: > > On Wed, May 20, 2026, at 04:01, Arseniy Mukhin wrote: > > One point - looks like the 0003 contains the same test as 0001. > > They are similar, but if you look carefully you'll see that they use > different injection points: > > 0001 uses async-notify-before-listen-commit > 0002 uses async-notify-after-listen-commit > Yes, but I meant 0001 and 0003. There are two specs: - async-notify-listen-startup.spec (added in 0001) - async-notify-listen-race.spec (added in 0003) Looks identical, unless I missed something. Best regards, Arseniy Mukhin ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: [PATCH] Fix LISTEN startup race with direct advancement 2026-05-19 20:37 [PATCH] Fix LISTEN startup race with direct advancement Joel Jacobson <[email protected]> 2026-05-20 11:01 ` Re: [PATCH] Fix LISTEN startup race with direct advancement Arseniy Mukhin <[email protected]> 2026-05-20 11:20 ` Re: [PATCH] Fix LISTEN startup race with direct advancement Joel Jacobson <[email protected]> 2026-05-20 17:27 ` Re: [PATCH] Fix LISTEN startup race with direct advancement Arseniy Mukhin <[email protected]> @ 2026-05-21 04:49 ` Joel Jacobson <[email protected]> 2026-05-26 15:40 ` Re: [PATCH] Fix LISTEN startup race with direct advancement Tom Lane <[email protected]> 0 siblings, 1 reply; 8+ messages in thread From: Joel Jacobson @ 2026-05-21 04:49 UTC (permalink / raw) To: Arseniy Mukhin <[email protected]>; +Cc: pgsql-hackers On Wed, May 20, 2026, at 10:27, Arseniy Mukhin wrote: > Yes, but I meant 0001 and 0003. There are two specs: > - async-notify-listen-startup.spec (added in 0001) > - async-notify-listen-race.spec (added in 0003) > Looks identical, unless I missed something. Ohh, right, thanks for spotting. New version attached, 0001 and 0002 identical, and 0003 now only contain the fix. /Joel Attachments: [application/octet-stream] v2-0001-Test-missed-LISTEN-startup-notification.patch (5.4K, 2-v2-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] v2-0003-Fix-LISTEN-startup-race-with-direct-advancement.patch (3.0K, 3-v2-0003-Fix-LISTEN-startup-race-with-direct-advancement.patch) download | inline diff: From 5c1fa5177b53779bcb8f200b19d66cb61a8b4208 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. --- src/backend/commands/async.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) 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; -- 2.52.0 [application/octet-stream] v2-0002-Test-LISTEN-startup-notification-for-already-seen-wo.patch (5.1K, 4-v2-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 ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: [PATCH] Fix LISTEN startup race with direct advancement 2026-05-19 20:37 [PATCH] Fix LISTEN startup race with direct advancement Joel Jacobson <[email protected]> 2026-05-20 11:01 ` Re: [PATCH] Fix LISTEN startup race with direct advancement Arseniy Mukhin <[email protected]> 2026-05-20 11:20 ` Re: [PATCH] Fix LISTEN startup race with direct advancement Joel Jacobson <[email protected]> 2026-05-20 17:27 ` Re: [PATCH] Fix LISTEN startup race with direct advancement Arseniy Mukhin <[email protected]> 2026-05-21 04:49 ` Re: [PATCH] Fix LISTEN startup race with direct advancement Joel Jacobson <[email protected]> @ 2026-05-26 15:40 ` Tom Lane <[email protected]> 2026-05-27 10:43 ` Re: [PATCH] Fix LISTEN startup race with direct advancement Joel Jacobson <[email protected]> 0 siblings, 1 reply; 8+ messages in thread From: Tom Lane @ 2026-05-26 15:40 UTC (permalink / raw) To: Joel Jacobson <[email protected]>; +Cc: Arseniy Mukhin <[email protected]>; pgsql-hackers "Joel Jacobson" <[email protected]> writes: > Ohh, right, thanks for spotting. New version attached, 0001 and 0002 identical, > and 0003 now only contain the fix. I agree with this fix, but it seems to me that it changes the meaning of the ListenerEntry.listening flag in a rather fundamental way. I'm tempted to rename that flag to "committed" or something like that. (Both "listening" and "committed" appear in dozens of places in this file that are not references to this flag, so TBH I'd rather use a flag name that is not either of those words. But I can't think of a better name.) Also, while the proposed test cases are good for showing that there's a bug here, I'm disinclined to commit them. I do not think there is value in them proportionate to the cost of two new isolation-test instances. regards, tom lane ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: [PATCH] Fix LISTEN startup race with direct advancement 2026-05-19 20:37 [PATCH] Fix LISTEN startup race with direct advancement Joel Jacobson <[email protected]> 2026-05-20 11:01 ` Re: [PATCH] Fix LISTEN startup race with direct advancement Arseniy Mukhin <[email protected]> 2026-05-20 11:20 ` Re: [PATCH] Fix LISTEN startup race with direct advancement Joel Jacobson <[email protected]> 2026-05-20 17:27 ` Re: [PATCH] Fix LISTEN startup race with direct advancement Arseniy Mukhin <[email protected]> 2026-05-21 04:49 ` Re: [PATCH] Fix LISTEN startup race with direct advancement Joel Jacobson <[email protected]> 2026-05-26 15:40 ` Re: [PATCH] Fix LISTEN startup race with direct advancement Tom Lane <[email protected]> @ 2026-05-27 10:43 ` Joel Jacobson <[email protected]> 2026-05-27 16:28 ` Re: [PATCH] Fix LISTEN startup race with direct advancement Tom Lane <[email protected]> 0 siblings, 1 reply; 8+ messages in thread From: Joel Jacobson @ 2026-05-27 10:43 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Arseniy Mukhin <[email protected]>; pgsql-hackers On Tue, May 26, 2026, at 17:40, Tom Lane wrote: > I agree with this fix, but it seems to me that it changes the meaning > of the ListenerEntry.listening flag in a rather fundamental way. > I'm tempted to rename that flag to "committed" or something like that. > > (Both "listening" and "committed" appear in dozens of places in this > file that are not references to this flag, so TBH I'd rather use a > flag name that is not either of those words. But I can't think of > a better name.) How about renaming listening to removeOnAbort and negating its meaning? > Also, while the proposed test cases are good for showing that there's > a bug here, I'm disinclined to commit them. I do not think there is > value in them proportionate to the cost of two new isolation-test > instances. I agree. I should have said that feel free to remove them. (Would be nice with a way to attach tests that are meant to be thrown away, but still let cfbot include them in testing.) /Joel Attachments: [application/octet-stream] v3-0001-Fix-NOTIFY-wakeups-for-pre-commit-LISTEN-entries.patch (6.7K, 2-v3-0001-Fix-NOTIFY-wakeups-for-pre-commit-LISTEN-entries.patch) download | inline diff: From 7fc2c6dfeb9eaf963aeb7db46fc9ae31db84cdf2 Mon Sep 17 00:00:00 2001 From: Joel Jacobson <[email protected]> Date: Wed, 27 May 2026 12:30:22 +0200 Subject: [PATCH] Fix NOTIFY wakeups for pre-commit LISTEN entries SignalBackends() used to ignore ListenerEntry entries whose flag said that the listener was not yet committed. That can be true for a LISTEN that has already registered its queue position, but has not yet reached AtCommit_Notify(). If another backend notifies the same channel in that window, advancing the listener queue pointer could make the LISTEN miss the notification after commit. Fix this by treating all channel entries as possible wakeup targets. Rename the flag to removeOnAbort to reflect its remaining purpose: identifying preallocated LISTEN entries that abort cleanup must remove. --- src/backend/commands/async.c | 54 +++++++++++++++--------------------- 1 file changed, 23 insertions(+), 31 deletions(-) diff --git a/src/backend/commands/async.c b/src/backend/commands/async.c index db6a9a6561b..1aae70303d0 100644 --- a/src/backend/commands/async.c +++ b/src/backend/commands/async.c @@ -114,15 +114,15 @@ * 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, marking new shared entries removeOnAbort. + * 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 - * deactivate listening. This happens before sending signals. + * applies the final per-channel listen/unlisten state. This happens + * before sending signals. * * SignalBackends() consults the shared global channel table to identify * listeners for the channels that the current transaction sent @@ -384,10 +384,9 @@ static SlruDesc NotifySlruDesc; * Global channel table definitions * * This hash table maps (database OID, channel name) keys to arrays of - * ProcNumbers representing the backends listening or about to listen - * on each channel. The "listening" flags allow us to create hash table - * entries pre-commit and not have to assume that creating them post-commit - * will succeed. + * ProcNumbers representing the backends listening or about to listen on each + * channel. The removeOnAbort flags allow us to create hash table entries + * pre-commit and discard them later if the transaction aborts. */ #define INITIAL_LISTENERS_ARRAY_SIZE 4 @@ -400,7 +399,7 @@ typedef struct GlobalChannelKey typedef struct ListenerEntry { ProcNumber procNo; /* listener's ProcNumber */ - bool listening; /* true if committed listener */ + bool removeOnAbort; /* remove entry if current xact aborts */ } ListenerEntry; typedef struct GlobalChannelEntry @@ -1523,9 +1522,8 @@ BecomeRegisteredListener(void) * * Prepare a LISTEN by recording it in pendingListenActions, pre-allocating * an entry in localChannelTable, and pre-allocating an entry in the shared - * globalChannelTable with listening=false. The listening flag will be set - * to true in AtCommit_Notify. If we abort later, unwanted table entries - * will be removed. + * globalChannelTable with removeOnAbort set. AtCommit_Notify clears + * removeOnAbort; abort processing removes entries still marked so. */ static void PrepareTableEntriesForListen(const char *channel) @@ -1557,7 +1555,7 @@ PrepareTableEntriesForListen(const char *channel) */ (void) hash_search(localChannelTable, channel, HASH_ENTER, NULL); - /* Pre-allocate entry in shared globalChannelTable with listening=false */ + /* Pre-allocate entry in shared globalChannelTable */ GlobalChannelKeyInit(&key, MyDatabaseId, channel); entry = dshash_find_or_insert(globalChannelTable, &key, &found); @@ -1592,7 +1590,7 @@ PrepareTableEntriesForListen(const char *channel) { if (listeners[i].procNo == MyProcNumber) { - /* Already have an entry; listening flag stays as-is until commit */ + /* Already have an entry; leave removeOnAbort as-is */ dshash_release_lock(globalChannelTable, entry); return; } @@ -1615,8 +1613,7 @@ PrepareTableEntriesForListen(const char *channel) } listeners[entry->numListeners].procNo = MyProcNumber; - listeners[entry->numListeners].listening = false; /* staged, not yet - * committed */ + listeners[entry->numListeners].removeOnAbort = true; entry->numListeners++; dshash_release_lock(globalChannelTable, entry); @@ -1766,11 +1763,10 @@ ApplyPendingListenActions(bool isCommit) if (pending->action == PENDING_LISTEN) { /* - * LISTEN being committed: set listening=true. - * localChannelTable entry was created during - * PreCommit and should be kept. + * LISTEN being committed; localChannelTable entry + * was created during PreCommit and should be kept. */ - listeners[i].listening = true; + listeners[i].removeOnAbort = false; removeLocal = false; } else @@ -1790,20 +1786,19 @@ ApplyPendingListenActions(bool isCommit) * pendingListenActions entries, so it's pretty hard to * test. */ - if (!listeners[i].listening) + if (listeners[i].removeOnAbort) { /* * Staged LISTEN (or LISTEN+UNLISTEN) being aborted, - * and we weren't listening before, so remove - * pre-allocated entries from both tables. + * so remove pre-allocated entries from both tables. */ RemoveListenerFromChannel(&entry, listeners, i); } else { /* - * We're aborting, but the previous state was that - * we're listening, so keep localChannelTable entry. + * Entry pre-existed this transaction, so keep the + * localChannelTable entry. */ removeLocal = false; } @@ -2304,9 +2299,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)) -- 2.52.0 ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: [PATCH] Fix LISTEN startup race with direct advancement 2026-05-19 20:37 [PATCH] Fix LISTEN startup race with direct advancement Joel Jacobson <[email protected]> 2026-05-20 11:01 ` Re: [PATCH] Fix LISTEN startup race with direct advancement Arseniy Mukhin <[email protected]> 2026-05-20 11:20 ` Re: [PATCH] Fix LISTEN startup race with direct advancement Joel Jacobson <[email protected]> 2026-05-20 17:27 ` Re: [PATCH] Fix LISTEN startup race with direct advancement Arseniy Mukhin <[email protected]> 2026-05-21 04:49 ` Re: [PATCH] Fix LISTEN startup race with direct advancement Joel Jacobson <[email protected]> 2026-05-26 15:40 ` Re: [PATCH] Fix LISTEN startup race with direct advancement Tom Lane <[email protected]> 2026-05-27 10:43 ` Re: [PATCH] Fix LISTEN startup race with direct advancement Joel Jacobson <[email protected]> @ 2026-05-27 16:28 ` Tom Lane <[email protected]> 0 siblings, 0 replies; 8+ messages in thread From: Tom Lane @ 2026-05-27 16:28 UTC (permalink / raw) To: Joel Jacobson <[email protected]>; +Cc: Arseniy Mukhin <[email protected]>; pgsql-hackers "Joel Jacobson" <[email protected]> writes: > On Tue, May 26, 2026, at 17:40, Tom Lane wrote: >> (Both "listening" and "committed" appear in dozens of places in this >> file that are not references to this flag, so TBH I'd rather use a >> flag name that is not either of those words. But I can't think of >> a better name.) > How about renaming listening to removeOnAbort and negating its meaning? Okay, or at least I haven't a better idea. Pushed after some more fiddling with the comments. regards, tom lane ^ permalink raw reply [nested|flat] 8+ messages in thread
end of thread, other threads:[~2026-05-27 16:28 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2026-05-19 20:37 [PATCH] Fix LISTEN startup race with direct advancement Joel Jacobson <[email protected]> 2026-05-20 11:01 ` Arseniy Mukhin <[email protected]> 2026-05-20 11:20 ` Joel Jacobson <[email protected]> 2026-05-20 17:27 ` Arseniy Mukhin <[email protected]> 2026-05-21 04:49 ` Joel Jacobson <[email protected]> 2026-05-26 15:40 ` Tom Lane <[email protected]> 2026-05-27 10:43 ` Joel Jacobson <[email protected]> 2026-05-27 16:28 ` Tom Lane <[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