Re: LISTEN/NOTIFY bug: VACUUM sets frozenxid past a xid in async queue

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?

--
Matheus Alcantara
EDB: http://www.enterprisedb.com