Обсуждение: LISTEN/NOTIFY bug: VACUUM sets frozenxid past a xid in async queue

On 2025-Aug-05, Alexandra Wang wrote:

> I'm bringing up a bug that was reported multiple times [1][2][3] in
> the bugs list here, for a broader audience.
> 
> The issue is that an ERROR like the one below occurs when trying to
> register any listener in the database.
> 
> test=# listen c21;
> ERROR:  58P01: could not access status of transaction 14279685
> DETAIL:  Could not open file "pg_xact/000D": No such file or directory.
> LOCATION:  SlruReportIOError, slru.c:1087

Oh, interesting problem.  Many thanks for the excellent write-up.

> My questions:
> 
> 1. Is it acceptable to drop notifications from the async queue if
> there are no active listeners? There might still be notifications that
> haven’t been read by any previous listener.

I'm somewhat wary of this idea -- could these inactive listeners become
active later and expect to be able to read their notifies?

> 2. If the answer to 1 is no, how can we teach VACUUM to respect the
> minimum xid stored in all AsyncQueueEntries?

Maybe we can have AsyncQueueAdvanceTail return the oldest XID of
listeners, and back off the pg_clog truncation based on that.  This
could be done by having a new boolean argument that says to look up the
XID from the PGPROC using BackendPidGetProc(QUEUE_BACKEND_PID) (which
would only be passed true by vac_update_datfrozenxid(), to avoid
overhead by other callers), then collect the oldest of those and return
it.

This does create the problem that an inactive listener could cause the
XID counter to stay far in the past.  Maybe we could try to avoid this
by adding more signalling (e.g, AsyncQueueAdvanceTail() itself could
send PROCSIG_NOTIFY_INTERRUPT signal?), and terminating backends that
are way overdue on reading notifies.  I'm not sure if this is really
needed or useful; consider a backend stuck on SIGSTOP (debugger or
whatever): it will just sit there forever.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

On Wed Aug 6, 2025 at 7:44 AM -03, Álvaro Herrera wrote:
>> My questions:
>>
>> 1. Is it acceptable to drop notifications from the async queue if
>> there are no active listeners? There might still be notifications that
>> haven’t been read by any previous listener.
>
> I'm somewhat wary of this idea -- could these inactive listeners become
> active later and expect to be able to read their notifies?
>
I'm bit worry about this too.

>> 2. If the answer to 1 is no, how can we teach VACUUM to respect the
>> minimum xid stored in all AsyncQueueEntries?
>
> Maybe we can have AsyncQueueAdvanceTail return the oldest XID of
> listeners, and back off the pg_clog truncation based on that.  This
> could be done by having a new boolean argument that says to look up the
> XID from the PGPROC using BackendPidGetProc(QUEUE_BACKEND_PID) (which
> would only be passed true by vac_update_datfrozenxid(), to avoid
> overhead by other callers), then collect the oldest of those and return
> it.
>
The problem with only considering the oldest XID of listeners is that
IIUC we may have notifications without listeners, and in this case we
may still get this error because when the LISTEN is executed we loop
through the AsyncQueueEntry's on asyncQueueProcessPageEntries() and we
call TransactionIdDidCommit() that raise the error before
IsListeningOn(channel) is called.

Another option would be to add a minXid field on AsyncQueueControl and
then update this value on asyncQueueProcessPageEntries() and
asyncQueueAddEntries() routines, and then we could check this value on
vac_update_datfrozenxid().

> This does create the problem that an inactive listener could cause the
> XID counter to stay far in the past.  Maybe we could try to avoid this
> by adding more signalling (e.g, AsyncQueueAdvanceTail() itself could
> send PROCSIG_NOTIFY_INTERRUPT signal?), and terminating backends that
> are way overdue on reading notifies.  I'm not sure if this is really
> needed or useful; consider a backend stuck on SIGSTOP (debugger or
> whatever): it will just sit there forever.
>
With this idea that I've proposed we still could have this problem, if a
listener take too long to consume a message we would block vacuum freeze
to advance the xid. For this I think that we could have two GUC's; One
to enable and disable the oldest xmin check on async queue and the
second to control how far we want to prevent the vacuum from freezing
the oldest async queue xid, and if the min xid raises this limit we
ignore and truncate the xid.

I've write a draft patch that plays with the idea, see attached.

--
Matheus Alcantara

Hi,

On Mon, Aug 11, 2025 at 8:41 PM Matheus Alcantara
<matheusssilv97@gmail.com> wrote:
>
> On Wed Aug 6, 2025 at 7:44 AM -03, Álvaro Herrera wrote:
> >> My questions:
> >>
> >> 1. Is it acceptable to drop notifications from the async queue if
> >> there are no active listeners? There might still be notifications that
> >> haven’t been read by any previous listener.
> >
> > I'm somewhat wary of this idea -- could these inactive listeners become
> > active later and expect to be able to read their notifies?
> >
> I'm bit worry about this too.

What exactly do we mean by "active listener"? According to the source code,
the active listener (as far as I understand) is the one who listens to at least
one channel. If we have no active listeners in the database, the new listener
will set its pointer to the tail of the async queue. Thus, messages with old
xid will not be touched by anybody. I don't see any point in dropping them
in this case.

If the "inactive" listener is the backend which is stuck somewhere, the
answer is "no" - this backend should be able to process all notifications.

>
> >> 2. If the answer to 1 is no, how can we teach VACUUM to respect the
> >> minimum xid stored in all AsyncQueueEntries?
> >
> > Maybe we can have AsyncQueueAdvanceTail return the oldest XID of
> > listeners, and back off the pg_clog truncation based on that.  This
> > could be done by having a new boolean argument that says to look up the
> > XID from the PGPROC using BackendPidGetProc(QUEUE_BACKEND_PID) (which
> > would only be passed true by vac_update_datfrozenxid(), to avoid
> > overhead by other callers), then collect the oldest of those and return
> > it.
> >
> The problem with only considering the oldest XID of listeners is that
> IIUC we may have notifications without listeners, and in this case we
> may still get this error because when the LISTEN is executed we loop
> through the AsyncQueueEntry's on asyncQueueProcessPageEntries() and we
> call TransactionIdDidCommit() that raise the error before
> IsListeningOn(channel) is called.
>

Agree.

> > This does create the problem that an inactive listener could cause the
> > XID counter to stay far in the past.  Maybe we could try to avoid this
> > by adding more signalling (e.g, AsyncQueueAdvanceTail() itself could
> > send PROCSIG_NOTIFY_INTERRUPT signal?), and terminating backends that
> > are way overdue on reading notifies.  I'm not sure if this is really
> > needed or useful; consider a backend stuck on SIGSTOP (debugger or
> > whatever): it will just sit there forever.
> >
> With this idea that I've proposed we still could have this problem, if a
> listener take too long to consume a message we would block vacuum freeze
> to advance the xid. For this I think that we could have two GUC's; One
> to enable and disable the oldest xmin check on async queue and the
> second to control how far we want to prevent the vacuum from freezing
> the oldest async queue xid, and if the min xid raises this limit we
> ignore and truncate the xid.
>

1) About Álvaro's comment:
I don't think that killing the lagging backend will save us, because entries
in the async queue are not ordered by xid. Thus, even after killing such a
backend, we have no guarantees that the problem is gone, because not
lagging backends still may encounter a super old xid.

2) About Matheus's comment:
I guess that there is only one reliable way to determine the precise minimal xid
in the queue (after message processing) - scan all pages from head to tail.
It obviously can take a lot of time. The second GUC (if I get it
right) logic is based
on minimal xid determination, so it won't be the best solution for
highload systems
(that are forced to turn on the first GUC, because of safety requirements).

> Another option would be to add a minXid field on AsyncQueueControl and
> then update this value on asyncQueueProcessPageEntries() and
> asyncQueueAddEntries() routines, and then we could check this value on
> vac_update_datfrozenxid().
>
> I've write a draft patch that plays with the idea, see attached.

Thanks for the patch! I have few comments on it :
1)
Maybe we should add an assert that NotifyQueueLock is held to the StoreEntryXid
function?

2)
For fields in AsyncQueueControl we have macros like QUEUE_HEAD,
QUEUE_TAIL and so on. Maybe minXid should also be wrapped with something
like QUEUE_MIN_XID?

3)
This logic will not work with (for example) two listeners. Steps :
session1=# listen c1;
session2=# listen c1;
session3=# notify c1; // sets minimal xid to N inside StoreEntryXid
session3=# notify c1;
session1=# listen c1; // sets minimal xid to (N + 1) inside ReleaseEntryXid

After these steps, session2 still needs to process a message with xid = N, but
autovacuum legally can freeze it and truncate clog.

Maybe we should check whether there are no listeners with smaller queue
position compared to the current backend?

4)
After ReleaseEntryXid, minXid field contains not actually minimal xid
in the queue,
but maximum processed xid. Maybe this parameter can be renamed?

--
Best regards,
Daniil Davydov

On Wed Aug 13, 2025 at 4:29 PM -03, Daniil Davydov wrote:
> Hi,
>
> On Mon, Aug 11, 2025 at 8:41 PM Matheus Alcantara
> <matheusssilv97@gmail.com> wrote:
>>
>> On Wed Aug 6, 2025 at 7:44 AM -03, Álvaro Herrera wrote:
>> >> My questions:
>> >>
>> >> 1. Is it acceptable to drop notifications from the async queue if
>> >> there are no active listeners? There might still be notifications that
>> >> haven’t been read by any previous listener.
>> >
>> > I'm somewhat wary of this idea -- could these inactive listeners become
>> > active later and expect to be able to read their notifies?
>> >
>> I'm bit worry about this too.
>
> What exactly do we mean by "active listener"? According to the source code,
> the active listener (as far as I understand) is the one who listens to at least
> one channel. If we have no active listeners in the database, the new listener
> will set its pointer to the tail of the async queue. Thus, messages with old
> xid will not be touched by anybody. I don't see any point in dropping them
> in this case.
>
I think that this definition is correct, but IIUC the tail can still
have notifications with xid's that were already truncated by vacuum
freeze. When the LISTEN is executed, we first loop through the
notification queue to try to advance the queue pointers and we can
eventually iterate over a notification that was added on the queue
without any listener but it has a xid that is already truncated by vacuum
freeze, so in this case it will fail to get the transaction status. On
Alex steps to reproduce the issue it first executes the NOTIFY and
then executes the LISTEN which fails after vacuum freeze.

> If the "inactive" listener is the backend which is stuck somewhere, the
> answer is "no" - this backend should be able to process all notifications.
>
I tried to reproduce the issue by using some kind of "inactive"
listener but so far I didn't manage to trigger the error. This is what I
tried:

1. Create listener:
postgres=# listen c1;

2. Execute a very long query to make the backend busy to process the
notification:
postgres=# select * from generate_series(1,10000000000) g where g > 1;

3. On another session send the notification
postgres=# notify c1;

4. Execute pgbench test:
pgbench -n -c 80 -j 10 -t 15000 -f test.sql postgres

5. Verify that we have multiple files on pg_xact:
➜  ls -lah ~/pg-devel/data/pg_xact
total 608
-rw-------@ 1 matheus  staff   256K Aug 18 20:36 0000
-rw-------@ 1 matheus  staff    40K Aug 18 20:56 0001

6. Execute VACUUM FREEZE on every database on the server

postgres=# VACUUM FREEZE;
VACUUM

postgres=# \c template1
You are now connected to database "template1" as user "postgres".
template1=# VACUUM FREEZE;
VACUUM

template1=# \c template0
You are now connected to database "template0" as user "postgres".
template0=# VACUUM FREEZE;
VACUUM

After the vacuum freeze I still can see the same files on pg_xact/ and
if I cancel the long query the notification is received correctly, and
then if I execute vacuum freeze again on every database the oldest
pg_xact file is truncated.

So, if my tests are correct I don't think that storing the oldest xid is
necessary anymore since I don't think that we can lose notifications
using the patch from Daniil or I'm missing something here?

Thinking about this, maybe another solution for this would be to change
queue advancing pointers to skip the transaction status check? With this
we would not need to touch any vacuum freeze code and instead change the
LISTEN execution to handle this scenario.

Thoughts?

--
Matheus Alcantara

Hi,

On Tue, Aug 19, 2025 at 7:14 AM Matheus Alcantara
<matheusssilv97@gmail.com> wrote:
>
> On Wed Aug 13, 2025 at 4:29 PM -03, Daniil Davydov wrote:
> >
> > What exactly do we mean by "active listener"? According to the source code,
> > the active listener (as far as I understand) is the one who listens to at least
> > one channel. If we have no active listeners in the database, the new listener
> > will set its pointer to the tail of the async queue. Thus, messages with old
> > xid will not be touched by anybody. I don't see any point in dropping them
> > in this case.
> >
> I think that this definition is correct, but IIUC the tail can still
> have notifications with xid's that were already truncated by vacuum
> freeze. When the LISTEN is executed, we first loop through the
> notification queue to try to advance the queue pointers and we can
> eventually iterate over a notification that was added on the queue
> without any listener but it has a xid that is already truncated by vacuum
> freeze, so in this case it will fail to get the transaction status. On
> Alex steps to reproduce the issue it first executes the NOTIFY and
> then executes the LISTEN which fails after vacuum freeze.
>

Yeah, you are right. I looked at the code again, and found out that even
if there are no active listeners, new listener should iterate from the head
to the tail. Thus, it may encounter truncated xid. Anyway, I still think that
dropping notifications is not the best way to resolve this issue.

> > If the "inactive" listener is the backend which is stuck somewhere, the
> > answer is "no" - this backend should be able to process all notifications.
> >
> I tried to reproduce the issue by using some kind of "inactive"
> listener but so far I didn't manage to trigger the error.
>
> After the vacuum freeze I still can see the same files on pg_xact/ and
> if I cancel the long query the notification is received correctly, and
> then if I execute vacuum freeze again on every database the oldest
> pg_xact file is truncated.
>
> So, if my tests are correct I don't think that storing the oldest xid is
> necessary anymore since I don't think that we can lose notifications
> using the patch from Daniil or I'm missing something here?
>

You have started a very long transaction, which holds its xid and prevents
vacuum from freezing it. But what if the backend is stuck not inside a
transaction? Maybe we can just hardcode a huge delay (not inside the
transaction) or stop process execution via breakpoint in gdb. If we will use it
instead of a long query, I think that this error may be reproducible.

--
Best regards,
Daniil Davydov

On Tue Aug 19, 2025 at 12:57 AM -03, Daniil Davydov wrote:
>> I think that this definition is correct, but IIUC the tail can still
>> have notifications with xid's that were already truncated by vacuum
>> freeze. When the LISTEN is executed, we first loop through the
>> notification queue to try to advance the queue pointers and we can
>> eventually iterate over a notification that was added on the queue
>> without any listener but it has a xid that is already truncated by vacuum
>> freeze, so in this case it will fail to get the transaction status. On
>> Alex steps to reproduce the issue it first executes the NOTIFY and
>> then executes the LISTEN which fails after vacuum freeze.
>>
>
> Yeah, you are right. I looked at the code again, and found out that even
> if there are no active listeners, new listener should iterate from the head
> to the tail. Thus, it may encounter truncated xid. Anyway, I still think that
> dropping notifications is not the best way to resolve this issue.
>
In the steps that Alex shared, is it expected that the "LISTEN c1" command
consumes the notification that was sent previously with NOTIFY? IIUC the
LISTEN command should be executed before of any NOTIFY, so executing the
LISTEN after a NOTIFY will not consume any previous notification added
on the channel, so how bad would be to drop this notification from the
queue in this situation?

>> > If the "inactive" listener is the backend which is stuck somewhere, the
>> > answer is "no" - this backend should be able to process all notifications.
>> >
>> I tried to reproduce the issue by using some kind of "inactive"
>> listener but so far I didn't manage to trigger the error.
>>
>> After the vacuum freeze I still can see the same files on pg_xact/ and
>> if I cancel the long query the notification is received correctly, and
>> then if I execute vacuum freeze again on every database the oldest
>> pg_xact file is truncated.
>>
>> So, if my tests are correct I don't think that storing the oldest xid is
>> necessary anymore since I don't think that we can lose notifications
>> using the patch from Daniil or I'm missing something here?
>>
>
> You have started a very long transaction, which holds its xid and prevents
> vacuum from freezing it. But what if the backend is stuck not inside a
> transaction? Maybe we can just hardcode a huge delay (not inside the
> transaction) or stop process execution via breakpoint in gdb. If we will use it
> instead of a long query, I think that this error may be reproducible.
>
But how could this happen in real scenarios? I mean, how the backend
could be stuck outside a transaction?

--
Matheus Alcantara

Hi,

On Tue, Aug 19, 2025 at 6:31 PM Matheus Alcantara
<matheusssilv97@gmail.com> wrote:
>
> On Tue Aug 19, 2025 at 12:57 AM -03, Daniil Davydov wrote:
> > You have started a very long transaction, which holds its xid and prevents
> > vacuum from freezing it. But what if the backend is stuck not inside a
> > transaction? Maybe we can just hardcode a huge delay (not inside the
> > transaction) or stop process execution via breakpoint in gdb. If we will use it
> > instead of a long query, I think that this error may be reproducible.
> >
> But how could this happen in real scenarios? I mean, how the backend
> could be stuck outside a transaction?
>

For now, I cannot come up with a situation where it may be possible.
Perhaps, such a lagging may occur during network communication,
but I couldn't reproduce it. Maybe other people know how we can achieve
this?

I think that if such a situation may be possible, the suggestion to  delete
messages  will no longer be relevant. Therefore, first of all, I would like to
clarify this issue.

--
Best regards,
Daniil Davydov

On Mon, Aug 18, 2025 at 5:14 PM Matheus Alcantara
<matheusssilv97@gmail.com> wrote:
>
> On Wed Aug 13, 2025 at 4:29 PM -03, Daniil Davydov wrote:
> > Hi,
> >
> > On Mon, Aug 11, 2025 at 8:41 PM Matheus Alcantara
> > <matheusssilv97@gmail.com> wrote:
> >>
> >> On Wed Aug 6, 2025 at 7:44 AM -03, Álvaro Herrera wrote:
> >> >> My questions:
> >> >>
> >> >> 1. Is it acceptable to drop notifications from the async queue if
> >> >> there are no active listeners? There might still be notifications that
> >> >> haven’t been read by any previous listener.
> >> >
> >> > I'm somewhat wary of this idea -- could these inactive listeners become
> >> > active later and expect to be able to read their notifies?
> >> >
> >> I'm bit worry about this too.
> >
> > What exactly do we mean by "active listener"? According to the source code,
> > the active listener (as far as I understand) is the one who listens to at least
> > one channel. If we have no active listeners in the database, the new listener
> > will set its pointer to the tail of the async queue. Thus, messages with old
> > xid will not be touched by anybody. I don't see any point in dropping them
> > in this case.
> >
> I think that this definition is correct, but IIUC the tail can still
> have notifications with xid's that were already truncated by vacuum
> freeze. When the LISTEN is executed, we first loop through the
> notification queue to try to advance the queue pointers and we can
> eventually iterate over a notification that was added on the queue
> without any listener but it has a xid that is already truncated by vacuum
> freeze, so in this case it will fail to get the transaction status.

When a process first executes the LISTEN command, it iterates through
the notification queue, but it seems only to advance its queue pointer
because it doesn't add the interested channels to its list yet and it
isn't interested in the notifications queued before it registered as a
listener. I'm wondering if we could optimize this by allowing the
queue pointer to fast-forward without checking transaction status. If
feasible, this might resolve the reported issue.

However, I have a more fundamental concern regarding the LISTEN/NOTIFY
implementation. Since vacuum doesn't consider the XIDs of notification
entries, there might be a critical issue with the truncation of clog
entries that LISTEN/NOTIFY still requires. As I understand it,
notification queue entries aren't ordered by XID, and it's possible
for a notification with an older XID to be positioned at the queue's
head. If vacuum freeze then truncates the corresponding clogs,
listeners attempting to retrieve this notification would fail to
obtain the transaction status. To address this, we likely need to
either implement Álvaro's suggestion[1] to make vacuum aware of the
oldest XID in the notification queue, or develop a mechanism to
remove/freeze XIDs of the notification entries.

Regards,

[1] https://www.postgresql.org/message-id/202508061044.ptcyt7aqsaaa%40alvherre.pgsql

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

On Tue Aug 19, 2025 at 2:37 PM -03, Daniil Davydov wrote:
> Hi,
>
> On Tue, Aug 19, 2025 at 6:31 PM Matheus Alcantara
> <matheusssilv97@gmail.com> wrote:
>>
>> On Tue Aug 19, 2025 at 12:57 AM -03, Daniil Davydov wrote:
>> > You have started a very long transaction, which holds its xid and prevents
>> > vacuum from freezing it. But what if the backend is stuck not inside a
>> > transaction? Maybe we can just hardcode a huge delay (not inside the
>> > transaction) or stop process execution via breakpoint in gdb. If we will use it
>> > instead of a long query, I think that this error may be reproducible.
>> >
>> But how could this happen in real scenarios? I mean, how the backend
>> could be stuck outside a transaction?
>>
>
> For now, I cannot come up with a situation where it may be possible.
> Perhaps, such a lagging may occur during network communication,
> but I couldn't reproduce it. Maybe other people know how we can achieve
> this?
>
Reading more the code I understand that once the a NOTIFY command is
received by a backend (and the transaction is committed) it will
emedialy signal all other listener backends and if the listener backend
is in idle it will consume the notification and then send it back to the
client as a PqMsg_NotificationResponse, so if there is a network delay
to send the notification from the listener backend back to the client I
don't think that it would be possible to get this error, because the
message was already dispatched by the backend and it will eventually get
to the client and once the notification is dispatched the backend
doesn't need to track it anymore (the queue pointers of the backend are
advanced after the dispatch).

Assuming that every SQL command is wrapped into a transaction (if it's
not already inside in) I think a busy listener backend will always
prevent the vacuum from freezing clog files past from its current xid,
so any notification that is sent while the backend is busy will not have
their transaction status removed from clog files anyway.

Is all these understandings and assumptions correct or am I missing
something here?

> I think that if such a situation may be possible, the suggestion to  delete
> messages  will no longer be relevant. Therefore, first of all, I would like to
> clarify this issue.
>
From what I've understood until now it seems to me that this can happen
only if we have a notification on the queue without any listener, so the
notification may stay on the queue from a long time and a vacuum freeze
can be executed during this time and then when we have a new listener
(even for a different channel) it will fail to advance the pointers at
listener setup(Exec_ListenPreCommit()) because it would not be able to
get the transition status of this very old notification.

(please note that I'm not trying to invalidate your concern, I'm also
have this concern but unfortunately I'm unable to reproduce it and I'm
just sharing my thoughts to see if this issue is really possible or not)

--
Matheus Alcantara

On Tue Aug 19, 2025 at 2:40 PM -03, Masahiko Sawada wrote:
> However, I have a more fundamental concern regarding the LISTEN/NOTIFY
> implementation. Since vacuum doesn't consider the XIDs of notification
> entries, there might be a critical issue with the truncation of clog
> entries that LISTEN/NOTIFY still requires. As I understand it,
> notification queue entries aren't ordered by XID, and it's possible
> for a notification with an older XID to be positioned at the queue's
> head. If vacuum freeze then truncates the corresponding clogs,
> listeners attempting to retrieve this notification would fail to
> obtain the transaction status. To address this, we likely need to
> either implement Álvaro's suggestion[1] to make vacuum aware of the
> oldest XID in the notification queue, or develop a mechanism to
> remove/freeze XIDs of the notification entries.
>
Thanks for the comments! Please see my reply at [1] that I mention that
I don't think that is too easy to have this specific scenario of a busy
backend loose dropped notifications.

[1] https://www.postgresql.org/message-id/DC7KGTXW3QSG.OZA24HONT78J%40gmail.com

--
Matheus Alcantara

On Wed, Aug 20, 2025 at 2:18 PM Matheus Alcantara
<matheusssilv97@gmail.com> wrote:
>
> On Tue Aug 19, 2025 at 2:37 PM -03, Daniil Davydov wrote:
> > Hi,
> >
> > On Tue, Aug 19, 2025 at 6:31 PM Matheus Alcantara
> > <matheusssilv97@gmail.com> wrote:
> >>
> >> On Tue Aug 19, 2025 at 12:57 AM -03, Daniil Davydov wrote:
> >> > You have started a very long transaction, which holds its xid and prevents
> >> > vacuum from freezing it. But what if the backend is stuck not inside a
> >> > transaction? Maybe we can just hardcode a huge delay (not inside the
> >> > transaction) or stop process execution via breakpoint in gdb. If we will use it
> >> > instead of a long query, I think that this error may be reproducible.
> >> >
> >> But how could this happen in real scenarios? I mean, how the backend
> >> could be stuck outside a transaction?
> >>
> >
> > For now, I cannot come up with a situation where it may be possible.
> > Perhaps, such a lagging may occur during network communication,
> > but I couldn't reproduce it. Maybe other people know how we can achieve
> > this?
> >
> Reading more the code I understand that once the a NOTIFY command is
> received by a backend (and the transaction is committed) it will
> emedialy signal all other listener backends and if the listener backend
> is in idle it will consume the notification and then send it back to the
> client as a PqMsg_NotificationResponse, so if there is a network delay
> to send the notification from the listener backend back to the client I
> don't think that it would be possible to get this error, because the
> message was already dispatched by the backend and it will eventually get
> to the client and once the notification is dispatched the backend
> doesn't need to track it anymore (the queue pointers of the backend are
> advanced after the dispatch).
>
> Assuming that every SQL command is wrapped into a transaction (if it's
> not already inside in) I think a busy listener backend will always
> prevent the vacuum from freezing clog files past from its current xid,
> so any notification that is sent while the backend is busy will not have
> their transaction status removed from clog files anyway.

What about backend processes that don't have any xid or xmin (i.e.,
are read-only query and in idle-in-transaction)?

IIUC we process the notification entries at the beginning of the
server loop (see L4608 in postgres.c) and when reading a command (via
ProcessClientReadInterrupt()), but it seems to me that if a process is
in idle-in-transaction state it doesn't process the entries unless the
transaction is committed. I've reproduced the missing clog entry error
even if we have a notification on the queue with a valid listener,
with the following steps:

1. Initialize the database cluster.

2. Execute "ALTER DATABASE template0 WITH ALLOW_CONNECTIONS true;".

3. Start one psql session and execute:

-- Session 1
=# listen s;
LISTEN
=# begin;
BEGIN

(keep this session open to leave the process idle-in-transaction.)

4. Open another psql session and execute:

-- Session 2
=# begin;
BEGIN
=# select txid_current();
 txid_current
--------------
          756
(1 row)
=# notify s;
NOTIFY
=# commit;
COMMIT

The notification to the channel 's' should be available for the
session-1's transaction.

5. Consume enough XIDs to truncate clog entries.

-- Session 2
=# create extension xid_wraparound;
CREATE EXTENSION
=# select consume_xids(10_000_000);
NOTICE:  consumed 10000000 / 10000000 XIDs, latest 0:10000757
 consume_xids
--------------
     10000757
(1 row)
=# select txid_current();
 txid_current
--------------
     10000758
(1 row)

6. Execute vacuum freeze on all databases:

$ vacuumdb --all --freeze
vacuumdb: vacuuming database "postgres"
vacuumdb: vacuuming database "template0"
vacuumdb: vacuuming database "template1"

$ psql -d postgres -c "select datname, datfrozenxid, age(datfrozenxid)
from pg_database"
  datname  | datfrozenxid | age
-----------+--------------+-----
 postgres  |     10000759 |  11
 template0 |     10000759 |  11
 template1 |     10000759 |  11
(3 rows)

7. On the first psql session:

-- Session 1
=# commit;
COMMIT
ERROR:  could not access status of transaction 756
DETAIL:  Could not open file "pg_xact/0000": No such file or directory.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

On Thu Aug 21, 2025 at 7:25 PM -03, Masahiko Sawada wrote:
> What about backend processes that don't have any xid or xmin (i.e.,
> are read-only query and in idle-in-transaction)?
>
> IIUC we process the notification entries at the beginning of the
> server loop (see L4608 in postgres.c) and when reading a command (via
> ProcessClientReadInterrupt()), but it seems to me that if a process is
> in idle-in-transaction state it doesn't process the entries unless the
> transaction is committed. I've reproduced the missing clog entry error
> even if we have a notification on the queue with a valid listener,
> with the following steps:
>
Ok, now we can reproduce this, thank you! So I think that we need a way
to teach the VACUUM FREEZE about old xid's on async queue.

I'll work on this considering the initial Daniil comments at [1]

[1] https://www.postgresql.org/message-id/CAJDiXgg1ArRB1-6wLtXfVVnQ38P9Y%2BCDfEc9M2TXiOf_4kfBMg%40mail.gmail.com

--
Matheus Alcantara

On Thu Aug 21, 2025 at 8:14 PM -03, Matheus Alcantara wrote:
> I'll work on this considering the initial Daniil comments at [1]
>
> [1] https://www.postgresql.org/message-id/CAJDiXgg1ArRB1-6wLtXfVVnQ38P9Y%2BCDfEc9M2TXiOf_4kfBMg%40mail.gmail.com
>
I've been working on this on the last few days, please see the attached
patch version.

In this new version I tried to follow the suggestion from Daniil of
scanning all pages from tail to head of the async queue.

I tried to use other data structures like a list or a hashmap to store
the xids but it shows to me more complicated due to duplicated
information being stored and also considering that this data structure
should be stored on shared memory, so I just tried to get the oldest xid
with the information that we already have on the async queue.

To find the oldest xid on the async queue I basically read all
AsyncQueueEntry's on async queue pages on SLRU cache from tail to head,
skipping only uncommitted transactions and notifications for different
databases.

The code that reads the pages and the entries within the page is a bit
coupled with the send notification logic. The important functions that
execute this is asyncQueueReadAllNotifications() which read the pages
from SLRU cache and then call the asyncQueueProcessPageEntries() which
read the entries within the page and send the notification to the client
if needed. Since I need a very similar code to read the notification
entries on the queue to check the min xid I think that it would be good
to have an API that just works like an iterator and returns the next
queue entry to work on so that it can be used for both cases.

In this patch I created an AsyncQueueIterator that iterates over pages
and AsyncQueueEntry's within the pages. The code is based on the
asyncQueueReadAllNotifications() and asyncQueueProcessPageEntries()
functions. For now I'm just implementing the iterator logic and use at
AsyncQueueMinXid() to get the min xid on the queue to make the review
more easier but I think that we can have another patch that refactor
asyncQueueReadAllNotifications() and asyncQueueProcessPageEntries()
functions to use this iterator approach and avoid duplicated code.

I'm also adding a TAP test that reproduces the issue, but I'm not sure if
it's declared on the best path.

I think that it would be good to perform some benchmark tests to see if
we have performance issues when reading very long async queues.

Thoughts?

--
Matheus Alcantara

I came across this discussion after we had this problem at least twice 
now at work.
I read through the discussion to see if this is the problem we are 
experiencing.
At first, I thought this was not exactly our problem, because of the way 
it was showed to be reproduced.

With a long running transaction that is listening, and then doing all 
the other steps like like running through transactions until a second 
pg_xact file is created, and then vacuuming etc etc...

But, I felt like we don't do this, so it cannot be the way we run into 
the problem. Specifically, having a listener as a long running transaction.


So I set out to get it into the broken state, and here is how I can 
reproduce it without long running queries:

1. Create another database

postgres=# CREATE DATABASE test;
CREATE DATABASE

2. Create a listener on the postgres database.

postgres=# LISTEN xx;
LISTEN

3. Create notifies for the test database.

Here I struggled to get a stuck notification in the queue (SELECT 
pg_notification_queue_usage();).

It can happen with only a single notify from a  psql connection, but I 
get a higher hit rate with the following:

Create a notify.sql with the only contents being:

```
NOTIFY s, 'yappers';
```
Then run this against the test database with pgbench.

pgbench -n -c 80 -j 20 -t 1000 -f notify.sql test


4. Confirm that there is now something stuck in the queue:

postgres=# SELECT pg_notification_queue_usage();
  pg_notification_queue_usage
-----------------------------
          9.5367431640625e-07
(1 row)

If this still shows 0, then run step 3 again.

5. Consume xid's. I create a file consume.sql with the only contents being:
```
SELECT txid_current();
```

Then:

pgbench -n -c 80 -j 30 -t 100000 -f consume.sql postgres

6. Verify that a new file is created in the pg_xact folder.

If not, just run the previous step again.

7. Run vacuum freeze. (Remember to allow connections to template0 
beforehand with `ALTER DATABASE template0 WITH ALLOW_CONNECTIONS true;`)

$ vacuumdb --all --freeze
vacuumdb: vacuuming database "postgres"
vacuumdb: vacuuming database "template0"
vacuumdb: vacuuming database "template1"
vacuumdb: vacuuming database "test"

8. Connect to the test database and try execute a listener.
test=# LISTEN anything;
ERROR:  could not access status of transaction 81086
DETAIL:  Could not open file "pg_xact/0000": No such file or directory.
test=#


Extra weirdness:

In step 2 create a connection to the test database and LISTEN there on 
any channel, even the one you are notifying to:
And you don't have to run anything on that connection, there is no 
backend_xmin on that connection, you don't start a transaction nothing, 
you just run `LISTEN something;`
Then after step 7, as long as you don't close this connection, you will 
not get an error when you try to set up a listener on the test database, 
even on a new connection to the test database.

What I'm saying is after step 7 you can have two connections. One to 
postgres database with active listener, and one to test database with 
active listener.
As long as the postgres one is open the queue is stuck, ie `SELECT 
pg_notification_queue_usage();` returns non-zero.
As long as the test database connection is open, new listeners do not 
experience the error.

If you close the test database connection, but leave the postgres 
connection, from now on any listener you try to create on test database 
will error.

If you close the postgres connection, the queue clears immediately and 
new LISTEN's on the test database will work.
This means it is possible to get rid of the problem without restarting 
the postmaster if you can close connections until you close the one that 
"caused" the problem.
Don't know if this is actually useful, but to me it seemed like everyone 
believed it to be impossible to recover without restarting, so it's 
something at least.


TLDR:
active listener on one database causes notify on another database to get 
stuck.
At no point could I get a stuck notify if I don't have a listener on at 
least one other database than the one I am notifying on. See the Extra 
weirdness section.
At no point do you need to have any other queries running, there is 
never an idle in transaction query needed for bad timing with the vacuum.

I hope I explained everything well enough so that one of you smart 
people can find and fix the problem.

29.08.2025 01:31, Matheus Alcantara пишет:
> On Thu Aug 21, 2025 at 8:14 PM -03, Matheus Alcantara wrote:
>> I'll work on this considering the initial Daniil comments at [1]
>>
>> [1] https://www.postgresql.org/message-id/CAJDiXgg1ArRB1-6wLtXfVVnQ38P9Y%2BCDfEc9M2TXiOf_4kfBMg%40mail.gmail.com
>>
> I've been working on this on the last few days, please see the attached
> patch version.
>
> In this new version I tried to follow the suggestion from Daniil of
> scanning all pages from tail to head of the async queue.

Since we still need to scan those pages, couldn't we mark finished
transactions as committed/aborted?
This way we may to not hold datfrozenxid for a long time and will allow
both safe clog truncation and safe async queue notification.
More over, most notifications could be marked as completed in
AtCommit_Notify already. So there is a need to recheck a few notifications
that were written but not marked in AtCommit_Notify.

Since AsyncQueueEntry field is used only to check visibility, looks like it
is safe to set it to FrozenTransactionId.

--
regards
Yura Sokolov aka funny-falcon

Hi,

On Tue, Sep 2, 2025 at 10:38 AM Yura Sokolov <y.sokolov@postgrespro.ru> wrote:
>
> 29.08.2025 01:31, Matheus Alcantara пишет:
> > On Thu Aug 21, 2025 at 8:14 PM -03, Matheus Alcantara wrote:
> >> I'll work on this considering the initial Daniil comments at [1]
> >>
> >> [1] https://www.postgresql.org/message-id/CAJDiXgg1ArRB1-6wLtXfVVnQ38P9Y%2BCDfEc9M2TXiOf_4kfBMg%40mail.gmail.com
> >>
> > I've been working on this on the last few days, please see the attached
> > patch version.
> >
> > In this new version I tried to follow the suggestion from Daniil of
> > scanning all pages from tail to head of the async queue.
>
> Since we still need to scan those pages, couldn't we mark finished
> transactions as committed/aborted?
> This way we may to not hold datfrozenxid for a long time and will allow
> both safe clog truncation and safe async queue notification.
> More over, most notifications could be marked as completed in
> AtCommit_Notify already. So there is a need to recheck a few notifications
> that were written but not marked in AtCommit_Notify.
>
> Since AsyncQueueEntry field is used only to check visibility, looks like it
> is safe to set it to FrozenTransactionId.

I like the idea to make queue records more self-sufficient, so we
don't need to check a transaction's status to determine if a
notification should be sent. What if we somehow mark every record of
an aborted transaction, so listeners can skip it without checking
transaction status? Also if the record is not marked as "aborted" we
also can send it without checking transaction status. IIUC
AtAbort_Notify is called before any listener can read records of the
aborted transaction (is it correct?), so we can do such "marking" in
AtAbort_Notify. And the listen/notify subsystem already has a similar
"marking" mechanism. At the end of every queue page we have a "dummy"
record which is skipped by every listener. Listeners skip it because
record's 'dboid' equals InvalidOid (listeners ignore any record where
the 'dboid' is different from their own). In the same way aborted
transactions can set 'dboid' to InvalidOid for their records in
AtAbort_Notify.

Some benefits of this approach:
1) Only aborted transactions would need to perform extra work.
2) Listeners don't need to check notify transaction status, but ofc
they still need to wait if the notify transaction is 'in progress'.
3) Vacuum don't need to be listen/notify aware.
4) We don't need to do an extra scan of the whole queue.

What do you think?


Best regards,
Arseniy Mukhin

On Mon Sep 1, 2025 at 11:06 AM -03, Jacques Combrink wrote:
> TLDR:
> active listener on one database causes notify on another database to get
> stuck.
> At no point could I get a stuck notify if I don't have a listener on at
> least one other database than the one I am notifying on. See the Extra
> weirdness section.
> At no point do you need to have any other queries running, there is
> never an idle in transaction query needed for bad timing with the vacuum.
>
> I hope I explained everything well enough so that one of you smart
> people can find and fix the problem.
>
The  long running transaction steps is just an example that we can lose
notifications using the first patch from Daniil that Alex has shared on
[1]. The steps that you've shared is just another way to trigger the
issue but it's similar to the steps that Alex also shared on [1].

All these different ways to trigger the error face the same underlying
problem: If a notification is keep for too long on the queue that vacuum
freeze can run and truncate clog files that contains transaction
information of this notification the error will happen.

The patch that I've attached on [2] aims to fix the issue following the
steps that you've shared, but during the tests I've found a stack
overflow bug on AsyncQueueIterNextNotification() due to the number of
notifications. I'm attaching a new version that fix this bug and I tried
to reproduce your steps with this new version and the issue seems to be
fixed.

Note that notifications that were added without any previous LISTEN will
block the xid advance during VACUUM FREEZE until we have a listener on
the database that owns these notifications. The XXX comment on vacuum.c
is about this problem.

[1] https://www.postgresql.org/message-id/CAK98qZ3wZLE-RZJN_Y%2BTFjiTRPPFPBwNBpBi5K5CU8hUHkzDpw%40mail.gmail.com
[2] https://www.postgresql.org/message-id CAFY6G8cJm73_MM9SuynZUqtqcaTuepUDgDuvS661oLW7U0dgsg%40mail.gmail.com

--
Matheus Alcantara

Thanks for the comments!

On Tue Sep 2, 2025 at 4:37 AM -03, Yura Sokolov wrote:
> 29.08.2025 01:31, Matheus Alcantara пишет:
>> On Thu Aug 21, 2025 at 8:14 PM -03, Matheus Alcantara wrote:
>>> I'll work on this considering the initial Daniil comments at [1]
>>>
>>> [1] https://www.postgresql.org/message-id/CAJDiXgg1ArRB1-6wLtXfVVnQ38P9Y%2BCDfEc9M2TXiOf_4kfBMg%40mail.gmail.com
>>>
>> I've been working on this on the last few days, please see the attached
>> patch version.
>>
>> In this new version I tried to follow the suggestion from Daniil of
>> scanning all pages from tail to head of the async queue.
>
> Since we still need to scan those pages, couldn't we mark finished
> transactions as committed/aborted?
> This way we may to not hold datfrozenxid for a long time and will allow
> both safe clog truncation and safe async queue notification.
> More over, most notifications could be marked as completed in
> AtCommit_Notify already. So there is a need to recheck a few notifications
> that were written but not marked in AtCommit_Notify.
>
> Since AsyncQueueEntry field is used only to check visibility, looks like it
> is safe to set it to FrozenTransactionId.
>
Maybe we could have a boolean "committed" field on AsyncQueueEntry and
check this field before sending the notification instead of call
TransactionIdDidCommit()? Is something similar what you are proposing?

We create the AsyncQueueEntry's and add into the SLRU at
PreCommit_Notify(), so to mark these entries as committed on
AtCommit_Notify() we would need a way to find these entries on the SLRU
from the pendingNotifies->events that contains the notifications added
on the current transaction being committed.

This idea looks promising to me TBH, I'm just not sure if it's
completely safe to mark an AsyncQueueEntry on the queue as committed
without checking on TransactionIdDidCommit(). I would like to read more
opinions about this before working on the next version based on this
idea.

--
Matheus Alcantara

On Wed Sep 3, 2025 at 4:28 PM -03, Arseniy Mukhin wrote:
>> Since we still need to scan those pages, couldn't we mark finished
>> transactions as committed/aborted?
>> This way we may to not hold datfrozenxid for a long time and will allow
>> both safe clog truncation and safe async queue notification.
>> More over, most notifications could be marked as completed in
>> AtCommit_Notify already. So there is a need to recheck a few notifications
>> that were written but not marked in AtCommit_Notify.
>>
>> Since AsyncQueueEntry field is used only to check visibility, looks like it
>> is safe to set it to FrozenTransactionId.
>
> I like the idea to make queue records more self-sufficient, so we
> don't need to check a transaction's status to determine if a
> notification should be sent. What if we somehow mark every record of
> an aborted transaction, so listeners can skip it without checking
> transaction status? Also if the record is not marked as "aborted" we
> also can send it without checking transaction status. IIUC
> AtAbort_Notify is called before any listener can read records of the
> aborted transaction (is it correct?), so we can do such "marking" in
> AtAbort_Notify. And the listen/notify subsystem already has a similar
> "marking" mechanism. At the end of every queue page we have a "dummy"
> record which is skipped by every listener. Listeners skip it because
> record's 'dboid' equals InvalidOid (listeners ignore any record where
> the 'dboid' is different from their own). In the same way aborted
> transactions can set 'dboid' to InvalidOid for their records in
> AtAbort_Notify.
>
> Some benefits of this approach:
> 1) Only aborted transactions would need to perform extra work.
> 2) Listeners don't need to check notify transaction status, but ofc
> they still need to wait if the notify transaction is 'in progress'.
> 3) Vacuum don't need to be listen/notify aware.
> 4) We don't need to do an extra scan of the whole queue.
>
> What do you think?
>
IIUC we don't store notifications of aborted transactions on the queue.
On PreCommit_Notify we add the notifications on the queue and on
Commit_Notify() we signal the backends.

Or I'm missing something here?

--
Matheus Alcantara

Hi,

On Thu, Sep 4, 2025 at 2:51 AM Rishu Bagga <rishu.postgres@gmail.com> wrote:
>
> On Wed, Sep 3, 2025 at 2:14 PM Matheus Alcantara
>
> <matheusssilv97@gmail.com> wrote:
>
>
> > IIUC we don't store notifications of aborted transactions on the
>
> > queue. On PreCommit_Notify we add the notifications on the queue
>
> > and on Commit_Notify() we signal the backends.
>
> >
>
> > Or I'm missing something here?
>
>
> My understanding is that something could go wrong in between
>
> PreCommit_Notify and AtCommit_Notify, which could cause the
>
> transaction to abort, and the notification might be in the queue at
>
> this point, even though the transaction aborted, hence the dependency
>
> on the commit log.

I have the same understanding.

>
> > Maybe we could have a boolean "committed" field on AsyncQueueEntry
>
> > and check this field before sending the notification instead of
>
> > call TransactionIdDidCommit()? Is something similar what you are
>
> > proposing?
>
>
> I like this direction, as it opens up the ability to remove the
>
> global lock held from PreCommit_Notify to the end of the transaction.
>
> In the same vein as this idea, I was experimenting with a patch
>
> inspired by Tom Lane’s idea in [1] where we write the actual
>
> notification data into the WAL, and just write the db, lsn, and xid
>
> into the notify queue in AtCommit_Notify, so the notify queue only
>
> contains information about committed transactions. Unfortunately,
>
> the additional WAL write overhead in this approach seemed to outweigh
>
> the benefits of removing the lock.


Interesting, have you shared your patch and results somewhere? IIUC
Tom's approach resolves this bug, because with it we have queue
entries produced by committed transactions only, so we don't need to
check their status and don't have dependency on clog.

>
> Following that idea - I think perhaps we could have two queues in the
>
> notify system, one that stores the notification data, and another
>
> that stores the position of the committed transaction’s notification,
>
> which we would append to in AtCommit_Notify. Then the listener would
>
> go through the position queue, and find / read the notifications from
>
> the notify data queue.
>

If I'm not wrong, by the time we got into AtCommit_Notify we no longer
hold the global lock, so we can't provide notifications in the commit
order. I think it would work if we add entries to the position queue
concurrently with adding commit entries to the wal, as in Tom's idea
3rd step.
Another question: how to truncate notification data queue. It sounds
like it will be more difficult to figure out what data in the queue is
still needed.


Best regards,
Arseniy Mukhin

On Thu, Sep 4, 2025 at 7:14 AM Arseniy Mukhin
<arseniy.mukhin.dev@gmail.com> wrote:

> Interesting, have you shared your patch and results somewhere? IIUC
> Tom's approach resolves this bug, because with it we have queue
> entries produced by committed transactions only, so we don't need to
> check their status and don't have dependency on clog.

I was eventually able to get better numbers with the patch after using
a substantial number of connections, so I have now posted it here. [1].

> Another question: how to truncate notification data queue. It sounds
> like it will be more difficult to figure out what data in the queue is
> still needed.

Yeah, I will think about this some more, and follow up with another
patch.

Thanks,
Rishu

[1] https://www.postgresql.org/message-id/CAK80%3DjipUfGC%2BU
QSzeA4oCP9daRtHZGm2SQZWLxC9NWmVTDtRQ%40mail.gmail.com

On Wed Sep 3, 2025 at 8:51 PM -03, Rishu Bagga wrote:
> On Wed, Sep 3, 2025 at 2:14 PM Matheus Alcantara
>
> <matheusssilv97@gmail.com> wrote:
>
>
>> IIUC we don't store notifications of aborted transactions on the
>
>> queue. On PreCommit_Notify we add the notifications on the queue
>
>> and on Commit_Notify() we signal the backends.
>
>>
>
>> Or I'm missing something here?
>
>
> My understanding is that something could go wrong in between
>
> PreCommit_Notify and AtCommit_Notify, which could cause the
>
> transaction to abort, and the notification might be in the queue at
>
> this point, even though the transaction aborted, hence the dependency
>
> on the commit log.
>
Ok, I agree that that this may happen but I don't see this as a common
case to fix the issue based on this behaviour. I think that we check the
transaction status also to skip notifications that were added on the
queue by transactions that are not fully committed yet, and I see this
scenario as a more common case but I could be wrong.

--
Matheus Alcantara

On Thu Sep 4, 2025 at 8:02 PM -03, Rishu Bagga wrote:
> On Thu, Sep 4, 2025 at 7:14 AM Arseniy Mukhin
> <arseniy.mukhin.dev@gmail.com> wrote:
>
>> Interesting, have you shared your patch and results somewhere? IIUC
>> Tom's approach resolves this bug, because with it we have queue
>> entries produced by committed transactions only, so we don't need to
>> check their status and don't have dependency on clog.
>
> I was eventually able to get better numbers with the patch after using
> a substantial number of connections, so I have now posted it here. [1].
>
Thanks for sharing the patch! I'll reserve some time to test and review.

--
Matheus Alcantara

On Fri, Sep 5, 2025 at 1:44 PM Matheus Alcantara
<matheusssilv97@gmail.com> wrote:
>
> On Wed Sep 3, 2025 at 8:51 PM -03, Rishu Bagga wrote:
> > On Wed, Sep 3, 2025 at 2:14 PM Matheus Alcantara
> >
> > <matheusssilv97@gmail.com> wrote:
> >
> >
> >> IIUC we don't store notifications of aborted transactions on the
> >
> >> queue. On PreCommit_Notify we add the notifications on the queue
> >
> >> and on Commit_Notify() we signal the backends.
> >
> >>
> >
> >> Or I'm missing something here?
> >
> >
> > My understanding is that something could go wrong in between
> >
> > PreCommit_Notify and AtCommit_Notify, which could cause the
> >
> > transaction to abort, and the notification might be in the queue at
> >
> > this point, even though the transaction aborted, hence the dependency
> >
> > on the commit log.
> >
> Ok, I agree that that this may happen but I don't see this as a common
> case to fix the issue based on this behaviour. I think that we check the
> transaction status also to skip notifications that were added on the
> queue by transactions that are not fully committed yet, and I see this
> scenario as a more common case but I could be wrong.
>

IIUC we don't need clog to check if the notification transaction is
still in progress, we use a snapshot for that (XidInMVCCSnapshot()).
If we see that the notification transaction is still in progress, we
postpone processing of that notification. If we see that notification
transaction is not in progress, then we check its status with
TransactionIdDidCommit() (using clog) and only then fail if clog
doesn't have notification transcation data (as a quick check, we can
see in the stack trace that the failure occurs during the call to
TransactionIdDidCommit, not XidInMVCCSnapshot).

So we need to check notification transaction status only to understand
if a transaction was committed and we can send notification or it was
aborted (somewhere in between PreCommit_Notify and AtCommit_Notify)
and we should skip notification. And the solution that Yura Sokolov
suggested is to introduce a 'commited' flag, so we can check it
instead of calling TransactionIdDidCommit. Another option is to make
aborted transactions to clean up after themselves in AtAbort_Notify(),
so if listener see notification in the queue and transaction that
wrote it is not in progress, then such notification was always created
by committed transaction and we also don't need to call
TransactionIdDidCommit. One way for aborted transactions to clean up
is to set its notifications dbOid to 0, so listeners skip them.
Another option I think is to set queue HEAD back to the position where
it was when the aborted transaction started to write to the queue. It
also makes such aborted transaction notifications 'invisible' for
listeners. We have a global lock and there is always only one writer,
so it sounds possible. And there is another option which is the patch
that Rishu Bagga is working on. In the patch all transactions write to
the listen/notify queue only after they wrote a commit record to the
WAL, so all notifications in the queue are always by committed
transactions and we don't need to call TransactionIdDidCommit.

Best regards,
Arseniy Mukhin

On Fri Sep 5, 2025 at 9:56 AM -03, Arseniy Mukhin wrote:
>> Ok, I agree that that this may happen but I don't see this as a common
>> case to fix the issue based on this behaviour. I think that we check the
>> transaction status also to skip notifications that were added on the
>> queue by transactions that are not fully committed yet, and I see this
>> scenario as a more common case but I could be wrong.
>>
>
> IIUC we don't need clog to check if the notification transaction is
> still in progress, we use a snapshot for that (XidInMVCCSnapshot()).
> If we see that the notification transaction is still in progress, we
> postpone processing of that notification. If we see that notification
> transaction is not in progress, then we check its status with
> TransactionIdDidCommit() (using clog) and only then fail if clog
> doesn't have notification transcation data (as a quick check, we can
> see in the stack trace that the failure occurs during the call to
> TransactionIdDidCommit, not XidInMVCCSnapshot).
>
> So we need to check notification transaction status only to understand
> if a transaction was committed and we can send notification or it was
> aborted (somewhere in between PreCommit_Notify and AtCommit_Notify)
> and we should skip notification. And the solution that Yura Sokolov
> suggested is to introduce a 'commited' flag, so we can check it
> instead of calling TransactionIdDidCommit. Another option is to make
> aborted transactions to clean up after themselves in AtAbort_Notify(),
> so if listener see notification in the queue and transaction that
> wrote it is not in progress, then such notification was always created
> by committed transaction and we also don't need to call
> TransactionIdDidCommit. One way for aborted transactions to clean up
> is to set its notifications dbOid to 0, so listeners skip them.
> Another option I think is to set queue HEAD back to the position where
> it was when the aborted transaction started to write to the queue. It
> also makes such aborted transaction notifications 'invisible' for
> listeners. We have a global lock and there is always only one writer,
> so it sounds possible. And there is another option which is the patch
> that Rishu Bagga is working on. In the patch all transactions write to
> the listen/notify queue only after they wrote a commit record to the
> WAL, so all notifications in the queue are always by committed
> transactions and we don't need to call TransactionIdDidCommit.
>
You are right, I missed the XidInMVCCSnapshot() call, sorry about that
and thanks very much for all the explanation!

I'll keep on hold the development of new patch versions for this thread
and focus on review and test the patch from Rishu at [1] to see if we
can make progress using the WAL approach.


[1] https://www.postgresql.org/message-id/CAK80%3DjipUfGC%2BUQSzeA4oCP9daRtHZGm2SQZWLxC9NWmVTDtRQ%40mail.gmail.com

--
Matheus Alcantara

On Sat, Sep 6, 2025 at 7:15 AM Matheus Alcantara
<matheusssilv97@gmail.com> wrote:
>
> On Fri Sep 5, 2025 at 9:56 AM -03, Arseniy Mukhin wrote:
> >> Ok, I agree that that this may happen but I don't see this as a common
> >> case to fix the issue based on this behaviour. I think that we check the
> >> transaction status also to skip notifications that were added on the
> >> queue by transactions that are not fully committed yet, and I see this
> >> scenario as a more common case but I could be wrong.
> >>
> >
> > IIUC we don't need clog to check if the notification transaction is
> > still in progress, we use a snapshot for that (XidInMVCCSnapshot()).
> > If we see that the notification transaction is still in progress, we
> > postpone processing of that notification. If we see that notification
> > transaction is not in progress, then we check its status with
> > TransactionIdDidCommit() (using clog) and only then fail if clog
> > doesn't have notification transcation data (as a quick check, we can
> > see in the stack trace that the failure occurs during the call to
> > TransactionIdDidCommit, not XidInMVCCSnapshot).
> >
> > So we need to check notification transaction status only to understand
> > if a transaction was committed and we can send notification or it was
> > aborted (somewhere in between PreCommit_Notify and AtCommit_Notify)
> > and we should skip notification. And the solution that Yura Sokolov
> > suggested is to introduce a 'commited' flag, so we can check it
> > instead of calling TransactionIdDidCommit. Another option is to make
> > aborted transactions to clean up after themselves in AtAbort_Notify(),
> > so if listener see notification in the queue and transaction that
> > wrote it is not in progress, then such notification was always created
> > by committed transaction and we also don't need to call
> > TransactionIdDidCommit. One way for aborted transactions to clean up
> > is to set its notifications dbOid to 0, so listeners skip them.
> > Another option I think is to set queue HEAD back to the position where
> > it was when the aborted transaction started to write to the queue. It
> > also makes such aborted transaction notifications 'invisible' for
> > listeners. We have a global lock and there is always only one writer,
> > so it sounds possible. And there is another option which is the patch
> > that Rishu Bagga is working on. In the patch all transactions write to
> > the listen/notify queue only after they wrote a commit record to the
> > WAL, so all notifications in the queue are always by committed
> > transactions and we don't need to call TransactionIdDidCommit.
> >
> You are right, I missed the XidInMVCCSnapshot() call, sorry about that
> and thanks very much for all the explanation!
>
> I'll keep on hold the development of new patch versions for this thread
> and focus on review and test the patch from Rishu at [1] to see if we
> can make progress using the WAL approach.

While the WAL-based approach discussed on another thread is promising,
I think it would not be acceptable for back branches as it requires
quite a lot of refactoring. Given that this is a long-standing bug in
listen/notify, I think we can continue discussing how to fix the issue
on backbranches on this thread.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

On Mon Sep 15, 2025 at 2:40 PM -03, Masahiko Sawada wrote:
> While the WAL-based approach discussed on another thread is promising,
> I think it would not be acceptable for back branches as it requires
> quite a lot of refactoring. Given that this is a long-standing bug in
> listen/notify, I think we can continue discussing how to fix the issue
> on backbranches on this thread.
>
Please see the new attached patch, it has a different implementation
that I've previously posted which is based on the idea that Arseniy
posted on [1].

This new version include the "committed" field on AsyncQueueEntry struct
so that we can use this info when processing the notification instead of
call TransactionIdDidCommit()

The "committed" field is set to true when the AsyncQueueEntry is being
added on the SLRU page buffer when the PreCommit_Notify() is called. If
an error occurs between the PreCommit_Notify() and AtCommit_Notify() the
AtAbort_Notify() will be called and will set the "committed" field to
false for the notifications inside the aborted transaction.

It's a bit tricky to know at AtAbort_Notify() which notifications were
added on the SLRU page buffer by the aborted transaction, so I created a
new data structure and a global variable to keep track of this
information. See the commit message for more information.

On the previously patch that I've posted I've created a TAP test to
reproduce the issue with the VACUUM FREEZE, this new version also
include this test and also a new test case that use the injection points
extension to force an error between the PreCommit_Notify() and
AtCommit_Notify() so that we can ensure that these notifications of an
aborted transaction are not visible to other listener backends.


[1] https://www.postgresql.org/message-id/CAE7r3M%2BXwf0A_aNhu7pYQd7nDQaqaEnyCjtvqg8XsgManmPOUA%40mail.gmail.com

--
Matheus Alcantara

Hi,

On Fri, Sep 19, 2025 at 12:35 AM Matheus Alcantara
<matheusssilv97@gmail.com> wrote:
>
> On Mon Sep 15, 2025 at 2:40 PM -03, Masahiko Sawada wrote:
> > While the WAL-based approach discussed on another thread is promising,
> > I think it would not be acceptable for back branches as it requires
> > quite a lot of refactoring. Given that this is a long-standing bug in
> > listen/notify, I think we can continue discussing how to fix the issue
> > on backbranches on this thread.
> >
> Please see the new attached patch, it has a different implementation
> that I've previously posted which is based on the idea that Arseniy
> posted on [1].
>

Thank you for the new version.

> This new version include the "committed" field on AsyncQueueEntry struct
> so that we can use this info when processing the notification instead of
> call TransactionIdDidCommit()
>
> The "committed" field is set to true when the AsyncQueueEntry is being
> added on the SLRU page buffer when the PreCommit_Notify() is called. If
> an error occurs between the PreCommit_Notify() and AtCommit_Notify() the
> AtAbort_Notify() will be called and will set the "committed" field to
> false for the notifications inside the aborted transaction.
>
> It's a bit tricky to know at AtAbort_Notify() which notifications were
> added on the SLRU page buffer by the aborted transaction, so I created a
> new data structure and a global variable to keep track of this
> information. See the commit message for more information.
>

I like this approach. We got rid of dependency on clog and don't limit
vacuum. Several points about the fix:

Is it correct to remember and reuse slru slots here? IIUC we can't do
it if we don't hold SLRU bank lock, because by the time we get in
AtAbort_Notify() the queue page could be already evicted. Probably we
need to use SimpleLruReadPage after we acquire the lock in
AtAbort_Notify()?

I think adding a boolean 'committed' is a good approach, but what do
you think about setting the queue head back to the position where
aborted transaction notifications start? We can do such a reset in
AtAbort_Notify(). So instead of marking notifications as
'commited=false' we completely erase them from the queue by moving the
queue head back. From listeners perspective if there is a notification
of completed transaction in the queue - it's always a committed
transaction, so again get rid of TransactionIdDidCommit() call. It
seems like a simpler approach because we don't need to remember all
notifications positions in the queue and don't need the additional
field 'committed'. All we need is to remember the head position before
we write anything to the queue, and reset it back if there is an
abort. IIUC Listeners will never send such erased notifications:
- while the aborted transaction is looking like 'in progress',
listeners can't send its notifications.
- by the time the aborted transaction is completed, the head is
already set back so erased notifications are located after the queue
head and listeners can't read it.

> On the previously patch that I've posted I've created a TAP test to
> reproduce the issue with the VACUUM FREEZE, this new version also
> include this test and also a new test case that use the injection points
> extension to force an error between the PreCommit_Notify() and
> AtCommit_Notify() so that we can ensure that these notifications of an
> aborted transaction are not visible to other listener backends.
>

I think it's a good test to have. FWIW there is a way to reproduce the
test condition without the injection point. We can use the fact that
serializable conflicts are checked after tx adds notifications to the
queue. Please find the attached patch with the example tap test. Not
sure if using injections points is more preferable?


Best regards,
Arseniy Mukhin

On Fri Sep 19, 2025 at 9:34 AM -03, Arseniy Mukhin wrote:
> Hi,
>
Thanks for taking a look at this!

> I like this approach. We got rid of dependency on clog and don't limit
> vacuum. Several points about the fix:
>
> Is it correct to remember and reuse slru slots here? IIUC we can't do
> it if we don't hold SLRU bank lock, because by the time we get in
> AtAbort_Notify() the queue page could be already evicted. Probably we
> need to use SimpleLruReadPage after we acquire the lock in
> AtAbort_Notify()?
>
Yeah, I think it make sense. Maybe we could just store the page number
and offset and call SimpleLruReadPage() inside AtAbort_Notify() to get
the slotno?

> I think adding a boolean 'committed' is a good approach, but what do
> you think about setting the queue head back to the position where
> aborted transaction notifications start? We can do such a reset in
> AtAbort_Notify(). So instead of marking notifications as
> 'commited=false' we completely erase them from the queue by moving the
> queue head back. From listeners perspective if there is a notification
> of completed transaction in the queue - it's always a committed
> transaction, so again get rid of TransactionIdDidCommit() call. It
> seems like a simpler approach because we don't need to remember all
> notifications positions in the queue and don't need the additional
> field 'committed'. All we need is to remember the head position before
> we write anything to the queue, and reset it back if there is an
> abort. IIUC Listeners will never send such erased notifications:
> - while the aborted transaction is looking like 'in progress',
> listeners can't send its notifications.
> - by the time the aborted transaction is completed, the head is
> already set back so erased notifications are located after the queue
> head and listeners can't read it.
>
I think that with this approach we may have a scenario where when we
enter the AtAbort_Notify() the queue head may not be the notification
from the transaction being aborted but it can be a notification from
another committed transaction, so resetting the queue head back to the
position before the aborted transaction would make us lose the
notification from the committed transaction. Is that make sense? What do
you think?

> I think it's a good test to have. FWIW there is a way to reproduce the
> test condition without the injection point. We can use the fact that
> serializable conflicts are checked after tx adds notifications to the
> queue. Please find the attached patch with the example tap test. Not
> sure if using injections points is more preferable?
>
Great, I think that if we can have a way to reproduce this without an
injection point would be better. I dind't look the test yet but I'll try
to incorporate this on the next patch version. Thanks!

--
Matheus Alcantara

On Fri, Sep 19, 2025 at 5:36 PM Matheus Alcantara
<matheusssilv97@gmail.com> wrote:
>
> On Fri Sep 19, 2025 at 9:34 AM -03, Arseniy Mukhin wrote:
> > Hi,
> >
> Thanks for taking a look at this!
>
> > I like this approach. We got rid of dependency on clog and don't limit
> > vacuum. Several points about the fix:
> >
> > Is it correct to remember and reuse slru slots here? IIUC we can't do
> > it if we don't hold SLRU bank lock, because by the time we get in
> > AtAbort_Notify() the queue page could be already evicted. Probably we
> > need to use SimpleLruReadPage after we acquire the lock in
> > AtAbort_Notify()?
> >
> Yeah, I think it make sense. Maybe we could just store the page number
> and offset and call SimpleLruReadPage() inside AtAbort_Notify() to get
> the slotno?

Yeah, I think it would work.

>
> > I think adding a boolean 'committed' is a good approach, but what do
> > you think about setting the queue head back to the position where
> > aborted transaction notifications start? We can do such a reset in
> > AtAbort_Notify(). So instead of marking notifications as
> > 'commited=false' we completely erase them from the queue by moving the
> > queue head back. From listeners perspective if there is a notification
> > of completed transaction in the queue - it's always a committed
> > transaction, so again get rid of TransactionIdDidCommit() call. It
> > seems like a simpler approach because we don't need to remember all
> > notifications positions in the queue and don't need the additional
> > field 'committed'. All we need is to remember the head position before
> > we write anything to the queue, and reset it back if there is an
> > abort. IIUC Listeners will never send such erased notifications:
> > - while the aborted transaction is looking like 'in progress',
> > listeners can't send its notifications.
> > - by the time the aborted transaction is completed, the head is
> > already set back so erased notifications are located after the queue
> > head and listeners can't read it.
> >
> I think that with this approach we may have a scenario where when we
> enter the AtAbort_Notify() the queue head may not be the notification
> from the transaction being aborted but it can be a notification from
> another committed transaction, so resetting the queue head back to the
> position before the aborted transaction would make us lose the
> notification from the committed transaction. Is that make sense? What do
> you think?

I think it's impossible: before pushing anything to the queue we
acquire global lock in PreCommit_Notify():

LockSharedObject(DatabaseRelationId, InvalidOid, 0, AccessExclusiveLock);

While we are holding the lock, no writers can add anything to the
queue. Then we save head position and add pending notifications to the
queue. The moment we get in AtAbort_Notify(), we still hold the global
lock (maybe it is worth adding Assert about it if we start relying on
it), so we can be sure there are no notifications in the queue after
the saved head position except ours. So it seems safe but maybe I
missed something.


Best regards,
Arseniy Mukhin

On Fri Sep 19, 2025 at 12:13 PM -03, Arseniy Mukhin wrote:
> I think it's impossible: before pushing anything to the queue we
> acquire global lock in PreCommit_Notify():
>
> LockSharedObject(DatabaseRelationId, InvalidOid, 0, AccessExclusiveLock);
>
> While we are holding the lock, no writers can add anything to the
> queue. Then we save head position and add pending notifications to the
> queue. The moment we get in AtAbort_Notify(), we still hold the global
> lock (maybe it is worth adding Assert about it if we start relying on
> it), so we can be sure there are no notifications in the queue after
> the saved head position except ours. So it seems safe but maybe I
> missed something.
>
Thanks for the explanation! I'm just not sure if I understand why do we
need the LWLockAcquire(NotifyQueueLock, LW_EXCLUSIVE) on
PreCommit_Notify() if we already have the
LockSharedObject(DatabaseRelationId, InvalidOid, 0, AccessExclusiveLock);

See the attached patch that is based on your the previous comment of
resetting the QUEUE_HEAD at AtAbort_Notify()

--
Matheus Alcantara

On Mon, Sep 22, 2025 at 4:09 PM Matheus Alcantara
<matheusssilv97@gmail.com> wrote:
>
> On Fri Sep 19, 2025 at 12:13 PM -03, Arseniy Mukhin wrote:
> > I think it's impossible: before pushing anything to the queue we
> > acquire global lock in PreCommit_Notify():
> >
> > LockSharedObject(DatabaseRelationId, InvalidOid, 0, AccessExclusiveLock);
> >
> > While we are holding the lock, no writers can add anything to the
> > queue. Then we save head position and add pending notifications to the
> > queue. The moment we get in AtAbort_Notify(), we still hold the global
> > lock (maybe it is worth adding Assert about it if we start relying on
> > it), so we can be sure there are no notifications in the queue after
> > the saved head position except ours. So it seems safe but maybe I
> > missed something.
> >
> Thanks for the explanation! I'm just not sure if I understand why do we
> need the LWLockAcquire(NotifyQueueLock, LW_EXCLUSIVE) on
> PreCommit_Notify() if we already have the
> LockSharedObject(DatabaseRelationId, InvalidOid, 0, AccessExclusiveLock);
>

Good question. It seems that it would be enough to hold
NotifyQueueLock only during the head update, so I don't understand it
either.

> See the attached patch that is based on your the previous comment of
> resetting the QUEUE_HEAD at AtAbort_Notify()

Thank you for the new version!

The fix looks exactly how I thought about it. But while thinking about
why we need to hold NotifyQueueLock in PreCommit_Notify I realized
there is a concurrency bug in the 'head resetting' approach. I thought
that listeners hold NotifyQueueLock lock while reading notifications
from the queue, but now I see that they hold it only while reading the
current head position. So we can end up in the next situation:

There are 2 backends:  listener and writer (backend with notifications)


        listener                                   writer
  ----------------------------------------------------------------------
                                         writer wants to commit, so it
                                         adds notifications to the queue
                                         in PreCommit_Notify() and advances
                                         queue head.

  ----------------------------------------------------------------------
listener wants to read notifications. It
gets into asyncQueueReadAllNotifications()
and reads the current head (it already
contains writer's notifications):


asyncQueueReadAllNotifications()
          ...
    LWLockAcquire(NotifyQueueLock, LW_SHARED);
    head = QUEUE_HEAD;
    LWLockRelease(NotifyQueueLock);
          ...

  ----------------------------------------------------------------------
                                         writer failed to commit, so it
                                         resets queue head in AtAbort_Notify()
                                         and completes the transaction.

  ----------------------------------------------------------------------
listener gets a snapshot where the writer
is not in progress.

        ...
    snapshot = RegisterSnapshot(GetLatestSnapshot());
        ...


This way the listener reads the head that includes all writer's
notifications and a snapshot where the writer is not in progress, so
nothing stops the listener from sending these notifications and it's
even possible to have the listener's position that is after the queue
head, so yes, it's bad :( Sorry about that.

Probably we can fix it (swap GetLatestSnapshot() with the head
position reading for example) but now I think that 'committed' field
approach is a more robust way to fix the bug. What do you think?

BTW one thing about 'committed' field fix version [0]. It seems that
instead of remembering all notification positions, we can remember the
head position after we acquire global lock and before we write
anything to the queue and in case of abort we can just start from the
saved position and mark all notifications until the end of the queue
as 'committed = false'. The reason is the same as with resetting the
head approach - as we hold global lock, no writers can add
notifications to the queue, so when we in AtAbort_Notify() all
notifications after the saved position are ours.


[0] https://www.postgresql.org/message-id/CAFY6G8fui7omKfUXF%2BJJ82B34ExrwK6J7vKe61S-DhEmr_jBhA%40mail.gmail.com


Best regards,
Arseniy Mukhin

On Tue Sep 23, 2025 at 1:11 PM -03, Arseniy Mukhin wrote:
>> Thanks for the explanation! I'm just not sure if I understand why do we
>> need the LWLockAcquire(NotifyQueueLock, LW_EXCLUSIVE) on
>> PreCommit_Notify() if we already have the
>> LockSharedObject(DatabaseRelationId, InvalidOid, 0, AccessExclusiveLock);
>>
>
> Good question. It seems that it would be enough to hold
> NotifyQueueLock only during the head update, so I don't understand it
> either.
>
IIUC correctly we acquire the LockSharedObject(DatabaseRelationId,
InvalidOid, 0, AccessExclusiveLock) to make other COMMIT's wait and the
LWLockAcquire(NotifyQueueLock, LW_EXCLUSIVE) to make listener backends
wait while we are adding the entries on the queue.

> Thank you for the new version!
>
> The fix looks exactly how I thought about it. But while thinking about
> why we need to hold NotifyQueueLock in PreCommit_Notify I realized
> there is a concurrency bug in the 'head resetting' approach. I thought
> that listeners hold NotifyQueueLock lock while reading notifications
> from the queue, but now I see that they hold it only while reading the
> current head position. So we can end up in the next situation:
>
> There are 2 backends:  listener and writer (backend with notifications)
>
>
>         listener                                   writer
>   ----------------------------------------------------------------------
>                                          writer wants to commit, so it
>                                          adds notifications to the queue
>                                          in PreCommit_Notify() and advances
>                                          queue head.
>
>   ----------------------------------------------------------------------
> listener wants to read notifications. It
> gets into asyncQueueReadAllNotifications()
> and reads the current head (it already
> contains writer's notifications):
>
>
> asyncQueueReadAllNotifications()
>           ...
>     LWLockAcquire(NotifyQueueLock, LW_SHARED);
>     head = QUEUE_HEAD;
>     LWLockRelease(NotifyQueueLock);
>           ...
>
>   ----------------------------------------------------------------------
>                                          writer failed to commit, so it
>                                          resets queue head in AtAbort_Notify()
>                                          and completes the transaction.
>
>   ----------------------------------------------------------------------
> listener gets a snapshot where the writer
> is not in progress.
>
>         ...
>     snapshot = RegisterSnapshot(GetLatestSnapshot());
>         ...
>
>
> This way the listener reads the head that includes all writer's
> notifications and a snapshot where the writer is not in progress, so
> nothing stops the listener from sending these notifications and it's
> even possible to have the listener's position that is after the queue
> head, so yes, it's bad :( Sorry about that.
>
Yeah, this is bad. I'm wondering if we could reproduce such race
conditions scenarios with some TAP tests.

> Probably we can fix it (swap GetLatestSnapshot() with the head
> position reading for example) but now I think that 'committed' field
> approach is a more robust way to fix the bug. What do you think?
>
I also agree that the committed field seems a more safe approach.

> BTW one thing about 'committed' field fix version [0]. It seems that
> instead of remembering all notification positions, we can remember the
> head position after we acquire global lock and before we write
> anything to the queue and in case of abort we can just start from the
> saved position and mark all notifications until the end of the queue
> as 'committed = false'. The reason is the same as with resetting the
> head approach - as we hold global lock, no writers can add
> notifications to the queue, so when we in AtAbort_Notify() all
> notifications after the saved position are ours.
>
See the new attached version which implements this idea of using the
committed field approach. I was just a bit concenerd about a race
condition situation where the QUEUE_HEAD is changed by another publisher
process and just iterating over all entries from the saved previous
QUEUE_HEAD position until the current QUEUE_HEAD position we could mark
successfully committed notifications as not committed by mistake, so in
this new patch version I save the QUEUE_HEAD position before we add the
entries on the shared queue and also the QUEUE_HEAD position after these
entries are added so we ensure that we only process the entries of this
range although we have the global lock
LockSharedObject(DatabaseRelationId) that may prevent this situation.

What do you think?

--
Matheus Alcantara

On Wed, Sep 24, 2025 at 1:40 AM Matheus Alcantara
<matheusssilv97@gmail.com> wrote:
>
> On Tue Sep 23, 2025 at 1:11 PM -03, Arseniy Mukhin wrote:
> >> Thanks for the explanation! I'm just not sure if I understand why do we
> >> need the LWLockAcquire(NotifyQueueLock, LW_EXCLUSIVE) on
> >> PreCommit_Notify() if we already have the
> >> LockSharedObject(DatabaseRelationId, InvalidOid, 0, AccessExclusiveLock);
> >>
> >
> > Good question. It seems that it would be enough to hold
> > NotifyQueueLock only during the head update, so I don't understand it
> > either.
> >
> IIUC correctly we acquire the LockSharedObject(DatabaseRelationId,
> InvalidOid, 0, AccessExclusiveLock) to make other COMMIT's wait and the
> LWLockAcquire(NotifyQueueLock, LW_EXCLUSIVE) to make listener backends
> wait while we are adding the entries on the queue.

Yeah, but I don't get why we want to block them with NotifyQueueLock
while we are writing to the queue. There is a SLRU bank lock and they
can't read anything after the head. Maybe it's just not a big deal?

> ...
> > This way the listener reads the head that includes all writer's
> > notifications and a snapshot where the writer is not in progress, so
> > nothing stops the listener from sending these notifications and it's
> > even possible to have the listener's position that is after the queue
> > head, so yes, it's bad :( Sorry about that.
> >
> Yeah, this is bad. I'm wondering if we could reproduce such race
> conditions scenarios with some TAP tests.
>

I agree it would be great to have more tests for such cases. As for
the 'committed field' patch, I think we can add a TAP test that shows
that listeners postpone processing of notifications until
notifications were marked as 'committed=false' in case of aborted
transactions. I tried to write one, but have not succeeded yet. Hope
to finish it soon.

> > Probably we can fix it (swap GetLatestSnapshot() with the head
> > position reading for example) but now I think that 'committed' field
> > approach is a more robust way to fix the bug. What do you think?
> >
> I also agree that the committed field seems a more safe approach.
>
> > BTW one thing about 'committed' field fix version [0]. It seems that
> > instead of remembering all notification positions, we can remember the
> > head position after we acquire global lock and before we write
> > anything to the queue and in case of abort we can just start from the
> > saved position and mark all notifications until the end of the queue
> > as 'committed = false'. The reason is the same as with resetting the
> > head approach - as we hold global lock, no writers can add
> > notifications to the queue, so when we in AtAbort_Notify() all
> > notifications after the saved position are ours.
> >
> See the new attached version which implements this idea of using the
> committed field approach. I was just a bit concenerd about a race
> condition situation where the QUEUE_HEAD is changed by another publisher
> process and just iterating over all entries from the saved previous
> QUEUE_HEAD position until the current QUEUE_HEAD position we could mark
> successfully committed notifications as not committed by mistake, so in
> this new patch version I save the QUEUE_HEAD position before we add the
> entries on the shared queue and also the QUEUE_HEAD position after these
> entries are added so we ensure that we only process the entries of this
> range although we have the global lock
> LockSharedObject(DatabaseRelationId) that may prevent this situation.

Thank you! Speaking of the scenario, my understanding is that it's
impossible as we hold the global lock, so QueuePosition head should
always be equal to QUEUE_HEAD when we get into At_AbortNotify(), but
maybe I'm wrong.

Patch looks great. Some minor points:

I have a warning when using git am with the patch:
     warning: 1 line adds whitespace errors.

There is a comment in the head of the async.c file about some
listen/notify internals. Maybe it's worth adding a comment about how
aborted transactions do clean up.

What do you think about a Assert in asyncQueueRollbackNotifications()
that other backends still see us as 'in progress'? So we can be sure
that they can't process our notifications before we mark notifications
as 'committed=false'. Not sure how to do it correctly, maybe

          Assert(TransactionIdIsValid(MyProc->xid));

will work? The TAP test that I tried to write also should test it.

There are several comments where the word "crash" is used. What do you
think about using "abort" instead? "Crash" sounds more like PANIC
situation where we don't care about notifications because they don't
survive restart.

Thank you!


Best regards,
Arseniy Mukhin

On Wed Sep 24, 2025 at 10:59 AM -03, Arseniy Mukhin wrote:
> Thank you! Speaking of the scenario, my understanding is that it's
> impossible as we hold the global lock, so QueuePosition head should
> always be equal to QUEUE_HEAD when we get into At_AbortNotify(), but
> maybe I'm wrong.
>
Let's see if anyone else has any thoughts about this.

> Patch looks great. Some minor points:
>
> I have a warning when using git am with the patch:
>      warning: 1 line adds whitespace errors.
>
I don't think that this is a problem? And I don't know how to remove
this warning, I've just created the patch with git format-patch @~1

> There is a comment in the head of the async.c file about some
> listen/notify internals. Maybe it's worth adding a comment about how
> aborted transactions do clean up.
>
Added

> What do you think about a Assert in asyncQueueRollbackNotifications()
> that other backends still see us as 'in progress'? So we can be sure
> that they can't process our notifications before we mark notifications
> as 'committed=false'. Not sure how to do it correctly, maybe
>
>           Assert(TransactionIdIsValid(MyProc->xid));
>
> will work? The TAP test that I tried to write also should test it.
>
Sounds resanable to me. I think that we could use the following assert
when iterating over the entries, what do you think?
            Assert(TransactionIdIsInProgress(qe->xid));

> There are several comments where the word "crash" is used. What do you
> think about using "abort" instead? "Crash" sounds more like PANIC
> situation where we don't care about notifications because they don't
> survive restart.
>
Good point, fixed

> Thank you!
>
Thanks for reviewing this!

--
Matheus Alcantara

On Wed, Sep 24, 2025 at 10:23 PM Matheus Alcantara
<matheusssilv97@gmail.com> wrote:
>
> On Wed Sep 24, 2025 at 10:59 AM -03, Arseniy Mukhin wrote:
> > ...
> > Patch looks great. Some minor points:
> >
> > I have a warning when using git am with the patch:
> >      warning: 1 line adds whitespace errors.
> >
> I don't think that this is a problem? And I don't know how to remove
> this warning, I've just created the patch with git format-patch @~1
>

In my experience a run of pgindent usually fixes all such warnings,
but I agree it's not a big deal.

>
> > What do you think about a Assert in asyncQueueRollbackNotifications()
> > that other backends still see us as 'in progress'? So we can be sure
> > that they can't process our notifications before we mark notifications
> > as 'committed=false'. Not sure how to do it correctly, maybe
> >
> >           Assert(TransactionIdIsValid(MyProc->xid));
> >
> > will work? The TAP test that I tried to write also should test it.
> >
> Sounds resanable to me. I think that we could use the following assert
> when iterating over the entries, what do you think?
>                         Assert(TransactionIdIsInProgress(qe->xid));
>

Yeah, TransactionIdIsInProgress() looks like the right way to do it.
And one more 'Assert' idea... maybe we can add
Assert(TransactionIdIsCurrentTransactionId(qe->xid)) just to verify
that we are only touching our own notifications?

Thank you, the fixes look good.


Best regards,
Arseniy Mukhin

Hi,

On Wed, Sep 24, 2025 at 4:59 PM Arseniy Mukhin
<arseniy.mukhin.dev@gmail.com> wrote:
>
> On Wed, Sep 24, 2025 at 1:40 AM Matheus Alcantara
> <matheusssilv97@gmail.com> wrote:
> > ...
> > > This way the listener reads the head that includes all writer's
> > > notifications and a snapshot where the writer is not in progress, so
> > > nothing stops the listener from sending these notifications and it's
> > > even possible to have the listener's position that is after the queue
> > > head, so yes, it's bad :( Sorry about that.
> > >
> > Yeah, this is bad. I'm wondering if we could reproduce such race
> > conditions scenarios with some TAP tests.
> >
>
> I agree it would be great to have more tests for such cases. As for
> the 'committed field' patch, I think we can add a TAP test that shows
> that listeners postpone processing of notifications until
> notifications were marked as 'committed=false' in case of aborted
> transactions. I tried to write one, but have not succeeded yet. Hope
> to finish it soon.

I finally managed to write a TAP test for it, so there is a new
version with the tap test.

I also realized that we can increase test coverage in
002_aborted_tx_notifies.pl if notifications of the aborted transaction
span several pages. This way we can better test
asyncQueueRollbackNotifications(). So I changed
002_aborted_tx_notifies.pl TAP test a bit.

And there is a small indentation change in lmgr.h that should fix this
git am warning.

I added all changes as a separate patch file (0002) so it was more
clear what changed. Please feel free to merge into the main patch file
/ drop any part of it that makes sense to you.


Best regards,
Arseniy Mukhin

On Sun Sep 28, 2025 at 10:17 AM -03, Arseniy Mukhin wrote:
>> I agree it would be great to have more tests for such cases. As for
>> the 'committed field' patch, I think we can add a TAP test that shows
>> that listeners postpone processing of notifications until
>> notifications were marked as 'committed=false' in case of aborted
>> transactions. I tried to write one, but have not succeeded yet. Hope
>> to finish it soon.
>
> I finally managed to write a TAP test for it, so there is a new
> version with the tap test.
>
> I also realized that we can increase test coverage in
> 002_aborted_tx_notifies.pl if notifications of the aborted transaction
> span several pages. This way we can better test
> asyncQueueRollbackNotifications(). So I changed
> 002_aborted_tx_notifies.pl TAP test a bit.
>
> And there is a small indentation change in lmgr.h that should fix this
> git am warning.
>
Thanks for the patches.

I've created a CF entry so we can get more reviews and comments:
https://commitfest.postgresql.org/patch/6095/

Let's see what other think about the approach being used to fix this
issue.

--
Matheus Alcantara

Hi,

On Wed, Oct 1, 2025 at 2:57 PM Matheus Alcantara
<matheusssilv97@gmail.com> wrote:
>
> On Sun Sep 28, 2025 at 10:17 AM -03, Arseniy Mukhin wrote:
> >> I agree it would be great to have more tests for such cases. As for
> >> the 'committed field' patch, I think we can add a TAP test that shows
> >> that listeners postpone processing of notifications until
> >> notifications were marked as 'committed=false' in case of aborted
> >> transactions. I tried to write one, but have not succeeded yet. Hope
> >> to finish it soon.
> >
> > I finally managed to write a TAP test for it, so there is a new
> > version with the tap test.
> >
> > I also realized that we can increase test coverage in
> > 002_aborted_tx_notifies.pl if notifications of the aborted transaction
> > span several pages. This way we can better test
> > asyncQueueRollbackNotifications(). So I changed
> > 002_aborted_tx_notifies.pl TAP test a bit.
> >
> > And there is a small indentation change in lmgr.h that should fix this
> > git am warning.
> >
> Thanks for the patches.
>
> I've created a CF entry so we can get more reviews and comments:
> https://commitfest.postgresql.org/patch/6095/
>

Thank you.

There is a test failure on CI, so please find the new patch version
with the fix (Makefile was updated a little bit). And I merged 0002
file with tap tests into 0001.


Best regards,
Arseniy Mukhin