Re: LISTEN/NOTIFY bug: VACUUM sets frozenxid past a xid in async queue - Mailing list pgsql-hackers

On Thu, Nov 6, 2025 at 3:06 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>
> On 06/11/2025 13:20, Arseniy Mukhin wrote:
> > On Thu, Nov 6, 2025 at 1:05 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> >>
> >> On 05/11/2025 21:02, Matheus Alcantara wrote:
> >>> On Wed Nov 5, 2025 at 9:59 AM -03, Heikki Linnakangas wrote:
> >>>>> In case of an error on TransactionIdDidCommit() I think that we will
> >>>>> have the same behavior as we advance the "current" position before of
> >>>>> DidCommit call the PG_FINALLY block will set the backend position past
> >>>>> the failing notification entry.
> >>>>
> >>>> With my patch, if TransactionIdDidCommit() fails, we will lose all the
> >>>> notifications that we have buffered in the local buffer but haven't
> >>>> passed to NotifyMyFrontEnd() yet. On 'master', you only lose a single
> >>>> notification, the one where TransactionIdDidCommit() failed.
> >>>>
> >>> Yeah, that's true.
> >>>
> >>>>> How bad would be to store the notification entries within a list and
> >>>>> store the next position with the notification entry and then wrap the
> >>>>> NotifyMyFrontEnd() call within a PG_TRY and update the "current" to the
> >>>>> saved "next position" on PG_CATCH? Something like this:
> >>>>
> >>>> [ ...]
> >>>>
> >>>> That addresses the failure on NotifyMyFrontEnd, but not on
> >>>> TransactionIdDidCommit.
> >>>>
> >>>> IMHO we should just make these errors FATAL. TransactionIdDidCommit()
> >>>> should not really fail, after fixing the bug we're discussing.
> >>>> NotifyMyFrontEnd() could fail on OOM, but that seems pretty unlikely on
> >>>> an otherwise idle connection. Or it could fail if the client connection
> >>>> is lost, but then the backend is about to die anyway. And arguably
> >>>> closing the connection is better than losing even a single notification,
> >>>> anyway.
> >>>>
> >>> My only concern with making these errors FATAL is that if a notification
> >>> entry causes a different, recoverable error, all subsequent messages
> >>> will be lost. This is because if backend die and the user open a new
> >>> connection and execute LISTEN on the channel it will not see these
> >>> notifications past the one that caused the error. I'm not sure if we are
> >>> completely safe from this case of a recoverable error, what do you
> >>> think?
> >>
> >> I did some more testing of the current behavior, using the encoding
> >> conversion to cause an error:
> >>
> >> In backend A:
> >>
> >> SET client_encoding='latin1';
> >> LISTEN foo;
> >>
> >> Backend b:
> >>
> >> NOTIFY foo, 'ハ';
> >>
> >> Observations:
> >>
> >> - If the connection is idle when the notification is received, the ERROR
> >> is turned into FATAL anyway:
> >>
> >> postgres=# SET client_encoding='latin1';
> >> SET
> >> postgres=# LISTEN foo;
> >> LISTEN
> >> postgres=# select 1; -- do the NOTIFY in another connection before this
> >> ERROR:  character with byte sequence 0xe3 0x83 0x8f in encoding "UTF8"
> >> has no equivalent in encoding "LATIN1"
> >> FATAL:  terminating connection because protocol synchronization was lost
> >> server closed the connection unexpectedly
> >>          This probably means the server terminated abnormally
> >>          before or while processing the request.
> >>
> >> - If there are multiple notifications pending, we stop processing the
> >> subsequent notifications on the first error. The subsequent
> >> notifications will only be processed when another notify interrupt is
> >> received, i.e. when a backend sends yet another notification.
> >>
> >> I'm getting more and more convinced that escalating all ERRORs to FATALs
> >> during notify processing is the right way to go. Attached is a new patch
> >> set that does that.
> >
> > One point about removing try/catch: when we execute LISTEN for the
> > very first time, we read the queue from the min(listener's pos) up to
> > the head. listenChannels is empty at the moment, so we never call
> > NotifyMyFrontEnd. But we do call TransactionIdDidCommit. So if we have
> > some problematic entry in the queue that we need to process during
> > initial LISTEN, it could block creation of new listeners. Is it
> > something we need to worry about?
>
> Hmm, I don't see how it could block the creation of new listeners.
> Exec_ListenPreCommit() starts from "max(listener's pos)" over existing
> listeners, not min(). Am I missing something?
>

My bad, you are right, we use max(listener's pos). But it doesn't
resolve the issue if I'm not missing something.

Let's say we have the queue:

(tail ... pos1 ... bad_entry_pos ... head)

bad_entry_pos - position of the entry where TransactionIdDidCommit fails.

We have the listener L1 with pos = pos1. It means every new listener
should process the queue from pos1 (as it's max(listener's pos)) up to
the queue head and when they try to do it, they will fail on
'bad_entry'.

> That said, Exec_ListenPreCommit() could start from 'min' among all
> listeners, if there are no listeners for the same backend. Per attached
> patch. I don't think it makes much difference though.
>

Yes, if there are no listeners, every new listener will start the
initial reading from the QUEUE_TAIL and will fail on the bad_entry
too.

But this example is valid only if we think we can fail on
TransactionIdDidCommit (even after the fix).

> Another small change we could make is to check for listenChannels == NIL
> before calling TransactionIdDidCommit.

There is a comment that describes why we need this initial reading in
Exec_ListenPreCommit:

    /*
     * This is our first LISTEN, so establish our pointer.
     *
     * We set our pointer to the global tail pointer and then move it forward
     * over already-committed notifications.  This ensures we cannot miss any
     * not-yet-committed notifications.  We might get a few more but that
     * doesn't hurt.

It seems that with such a check we will skip not-yet-committed
notifications and always get to the HEAD. If we decide that it's ok,
maybe we can just set 'pos' of every new listener to the HEAD without
reading?


Best regards,
Arseniy Mukhin