From: | "Joel Jacobson" <joel(at)compiler(dot)org> |
---|---|
To: | "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | "Matheus Alcantara" <matheusssilv97(at)gmail(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Optimize LISTEN/NOTIFY |
Date: | 2025-10-08 14:31:24 |
Message-ID: | 474efa78-337c-41cd-a73a-f845a0115109@app.fastmail.com |
Views: | Whole Thread | Raw Message | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Tue, Oct 7, 2025, at 22:15, Tom Lane wrote:
> "Joel Jacobson" <joel(at)compiler(dot)org> writes:
>> Ops, I see I got the list_member() code wrong. I've changed it to now
>> create String nodes, and then use strVal().
>
> Might be better to revert to the previous coding. Using String
> nodes is going to roughly double the space eaten for the list,
> and it seems like it's not buying you a lot.
>
>> I also changed back to dshash_find(..., false) in SignalBackends(),
>> since that makes more sense to me, since we're not modifying entry.
>
> Agreed.
>
> I did a code coverage run and it seems like things are in pretty
> good shape already. async.c is about 88% covered and a lot of the
> omissions are either Trace_notify or unreached error reports, which
> I'm not especially concerned about. The visible coverage gaps are:
>
> 1. asyncQueueFillWarning. This wasn't covered before either, because
> it doesn't seem very practical to exercise it in an everyday
> regression test. Since your patch doesn't touch that code nor the
> queue contents, I'm not concerned here.
I agree.
> 2. AtSubCommit_Notify's reparenting stanza. This is a pre-existing
> omission too, but maybe worth doing something about?
>
> 3. AtSubAbort_Notify's pendingActions cleanup loop; same comments.
>
> 4. notification_match is not called at all. Again, pre-existing
> coverage gap.
I've added test coverage for all three items above.
> 5. ChannelHashAddListener: "already registered" case is not reached,
> which surprises me a bit, and neither is the "grow the array" stanza.
> Since this is new code it might be worth adding coverage.
I've added a test for the "grow the array" stanza.
The "already registered" case seems impossible to reach, since the
caller, Exec_ListenCommit, returns early if IsListeningOn.
Patches:
0001-optimize_listen_notify-v12.patch:
Improve LISTEN/NOTIFY test coverage
0002-optimize_listen_notify-v12.patch:
Optimize LISTEN/NOTIFY with channel-specific listener tracking
I split this into two patches, to make it easier to verify that the
second patch doesn't affect the tests added by the first patch. The 0001
patch also includes the "grow the array" test, which is pointless
without the 0002 patch, but felt better to add it first anyway.
I've also made changes in v12 based on feedback from Chao Li, to which I
will reply to shortly.
/Joel
Attachment | Content-Type | Size |
---|---|---|
0001-optimize_listen_notify-v12.patch | application/octet-stream | 7.8 KB |
0002-optimize_listen_notify-v12.patch | application/octet-stream | 22.4 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Robert Haas | 2025-10-08 14:39:14 | Re: Thoughts on a "global" client configuration? |
Previous Message | Aleksander Alekseev | 2025-10-08 14:30:58 | Re: [PATCH] Remove unused #include's in src/backend/commands/* |