Re: Proposal: Out-of-Order NOTIFY via GUC to Improve LISTEN/NOTIFY Throughput

Hi Joel, Arseniy, Matheus, thanks for taking a look. I’ve attached an
updated patch and rebased on the latest commits that fixes the
correctness issues.

On Fri, Sep 5, 2025 at 2:38 AM Joel Jacobson <joel(at)compiler(dot)org> wrote:
> What's the definition of the test table?

It’s just a one column integer table defined as such:

CREATE TABLE test(x int);

> In your benchmark, how many backends were doing
> `LISTEN benchmark_channel;`?

I just tested the notify codepath throughput, with no listeners.

> It would be interesting if you could run the benchmark and vary the
> number of listeners, e.g. with 1, 10, 100 listeners, to understand the
> effect of this patch for different type of workloads.

IIUC, the current benchmark wouldn’t be affected by listeners - the
commit of the notifying transaction is independent from the listener
reading the transaction. I agree that it would be useful to test listener
performance as well. I will work on this shortly.

On Fri, Sep 5, 2025 at 10:31 AM Arseniy Mukhin
<arseniy(dot)mukhin(dot)dev(at)gmail(dot)com> wrote:

> 1) There are some compilation warnings about unused functions.

Taken care of in the attached patch.

> 2) I thought the idea was to add notification_data_entry_lsn to the
> transaction's commit wal entry, but the patch introduced the new
> XLOG_ASYNC_NOTIFY_COMMIT wal entry and put lsn in it. Also
> XLOG_ASYNC_NOTIFY_COMMIT is written in AtCommit_Notify, when a
> transaction is already committed, so we write it after the commit
> entry. I don't think we can write anything about the transaction to
> the wal after we wrote the transaction commit record. In my
> understanding all transaction stuff in wal should be before commit /
> abort entries. At least because during committing we sync wal with
> disk up to the commit entry, and we can't guarantee that anything
> after a commit entry will survive a crash. Anyway I think we just
> don't need this new wal entry.

I removed the XLOG_ASYNC_NOTIFY_COMMIT record in the new patch,
in favor of including the LSN of the notification as a new field in the
commit record itself. The reasoning here is that Notify data is WAL’d
before transaction commit, so during WAL replay, we need some way
to know whether the notification should be added to the notify queue.
In the previous patch we were using XLOG_ASYNC_NOTIFY_COMMIT
record, but actually this doesn’t work, because we could write this
record before commit, or we could commit and crash before writing
this record. So the best solution is to have a field within the commit
record, that tells us which log record contains the notifications for that
transaction, if there are any.

> I see you removed global lock, but I don't see where you use lock
> for step 3 [0] to provide the commit order of notifications.

Fixed this in the updated patch.

On Sat, Sep 6, 2025 at 7:52 AM Matheus Alcantara
<matheusssilv97(at)gmail(dot)com> wrote:

> Your patch already aims to fix the issue? On [2] I implemented a TAP
> test that reproduce the issue and I tried to execute using your patch
> and I still see the error. I'm attaching the TAP test isolated and
> maybe we could incorporate into your patch series to ensure that the
> issue is fixed? What do you think?

I wasn’t able to run the TAP tests; however, in the updated patch, we
can be confident that entries in the queue are from committed
transactions. If there is a crash before committing and after writing to
the queue, this would be within the critical section, so a notification
from an uncommitted transaction would never be read in the queue.
That allows us to remove the XidInMVCCSnapshot and
TransactionIdDidCommit check.

Thanks,
Rishu