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

On 04/11/2025 16:40, Arseniy Mukhin wrote:
> It seems that 0002 handles errors during NotifyMyFrontEnd a little differently.
>
> With master, in case of a failure during NotifyMyFrontEnd, the
> listener's position in PG_FINALLY is set to the beginning of the next
> notification, since we advance the "current" position only if the
> previous notification was successfully sent.
>
> With 0002, we advance the "current" position while copying
> notifications to the local buffer, and begin sending them after the
> position has already been advanced for all copied notifications. So in
> case of a failure, the listener's position in PG_FINALLY is set to the
> beginning of the next page or queue head. This means we can lose
> notifications that were copied but were not sent.

True.

> If we want to preserve the previous behavior, maybe we could use a new
> local position while copying notifications and only advance the
> "current" position while sending notifications to the frontend?

That's not good. The loop advances 'current' before calling
TransactionIdDidCommit() to ensure that if there's a broken entry in the
queue for which TransactionIdDidCommit() throws an error, we advance
'current' past that point. Otherwise we would get back later to try to
process the same broken entry again and again.

We could put the NotifyMyFrontEnd() calls in a PG_FINALLY block, so that
the copied notifications get sent even on error. But I'm a reluctant to
put that in PG_FINALLY, it's not clear to me if NotifyMyFrontEnd() is
safe to call during error processing. And if the client is not
processing the notifications, the abort processing could be delayed for
a long time.

One idea is to close the client connection on a TransactionIdDidCommit
error, i.e. make the error FATAL. The current behavior where we skip the
notification seems problematic already. Closing the connection would be
a clear signal that some notifications have been lost.

Or we can just accept new behavior that if TransactionIdDidCommit()
fails, we might lose more notifications than the one that caused the error.

- Heikki