public inbox for [email protected]
help / color / mirror / Atom feedFrom: Joel Jacobson <[email protected]>
To: Tom Lane <[email protected]>
Cc: Chao Li <[email protected]>
Cc: pgsql-hackers <[email protected]>
Subject: Re: Optimize LISTEN/NOTIFY
Date: Sun, 23 Nov 2025 16:49:26 +0100
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<CAK80=jhmE40KVqQ3ho37MArS7cAED1p9m7uikDxcnDmqdW7t8A@mail.gmail.com>
<[email protected]>
<[email protected]>
<CA+hUKGLrMGkWDB0cwTa0RqD+AF7O-Ywgck8aVYKwOQnZgYRRug@mail.gmail.com>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<CAFY6G8dap-bCnAnMG-2Gzew8yv2Vbi9gsx9+yszKMmd57ygfvA@mail.gmail.com>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<CAE7r3MLivh1sHWF06hrVXkiQbw-KChPcQsh+9CheXprm5vRVMQ@mail.gmail.com>
<14865EB6-0BF4-462B-9072-10BDAC10C0>
<[email protected]>
<[email protected]>
On Sat, Nov 22, 2025, at 22:30, Joel Jacobson wrote:
> On Thu, Nov 20, 2025, at 21:26, Tom Lane wrote:
>> I took a brief look through the v28 patch, and I'm fairly distressed
>> at how much new logic has been stuffed into what's effectively a
>> critical section. It's totally not okay for AtCommit_Notify or
>> anything it calls to throw an error
>
> Right, I agree. Thanks for guidance.
>
>> So I think there needs to be a serious effort made to move as
>> much as we possibly can of the potentially-risky stuff into
>> PreCommit_Notify. In particular I think we ought to create
>> the shared channel hash entry then, and even insert our PID
>> into it. We could expand the listenersArray entries to include
>> both a PID and a boolean "is it REALLY listening?", and then
>> during Exec_ListenCommit we'd only be required to find an
>> entry we already added and set its boolean, so there's no OOM
>> hazard. Possibly do something similar with the local
>> listenChannelsHash, so as to remove that possibility of OOM
>> failure as well.
>
> Thanks for the idea, I like this approach. I've expanded the
> listenersArray like suggested, and moved all risky stuff from
> Exec_ListenCommit to PreCommit_Notify.
>
>> (An alternative design could be to go ahead and insert our
>> PID during PreCommit_Notify, and just tolerate the small
>> risk of getting signaled when we didn't need to be. But
>> then we'd need some mechanism for cleaning out the bogus
>> entry during AtAbort_Notify.)
>
> We seem to need a cleanup mechanism also with the boolean field design,
> since a channel could end up being added only to listenChannelsHash, but
> not channelHash, which would trick IsListeningOn() into falsely thinking
> we're listening on such channel when we're not. This could happen if
> successfully adding the channel to listenChannelsHash, but OOM when
> trying to add it to channelHash.
>
> AtAbort_Notify now handles such half-state, by reconciling all channels
> that had LISTEN_LISTEN actions, using the channelHash as the single
> source of truth, removing channels from both listenChannelsHash and
> channelHash, unless the active field is true (which means we were
> already listening to the channel due to a previous transaction).
>
> I've tested triggering the cleanup logic by adding elog ERROR that
> triggered after listenChannelsHash insert, and another test that
> triggered after channelHash insert, and it cleaned it up correctly. I
> haven't created a test for it in tree though, would we want that?
>
>> I'm not sure what I think about the new logic in SignalBackends
>> from this standpoint. Making it very-low-probability-of-error
>> definitely needs some consideration though.
>
> The initChannelHash call in SignalBackends is now gone, moved to
> PreCommit_Notify (called if there are any pendingNotifies).
>
> I also took the liberty of fixing the XXX comment, to lazily preallocate
> the signals arrays during PreCommit_Notify. It felt too inconsistent to
> just leave that unfixed, but maybe should be a separate commit?
I've extracted the preallocation of signals arrays into a separate patch:
https://commitfest.postgresql.org/patch/6248/
> I wonder how risky the remaining new logic in SignalBackends is. For
> instance, I looked at dshash_find(..., false), and note it calls
> LWLockAcquire which in turn could elog ERROR if num locks is exceeded,
> but in master we're already calling LWLockAcquire in SignalBackends, so
> should be fine I guess?
>
> Apart from that, I don't see any new logic in SignalBackends, that could
> potentially be risky.
/Joel
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]
Subject: Re: Optimize LISTEN/NOTIFY
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