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

On 05/11/2025 14:40, Matheus Alcantara wrote:
> On Wed Nov 5, 2025 at 6:21 AM -03, Heikki Linnakangas wrote:
>> On 04/11/2025 16:40, Arseniy Mukhin wrote:
>>> 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.
>>
> I think that we may have an error on TransactionIdDidCommit() and on
> NotifyMyFrontEnd() right?

True.

> 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.

> I think that the problem is on NotifyMyFrontEnd() call that if it fails
> we will set the current backend position to the head of the queue
> or the beginning of the next page possibly losing notifications.

True, a failure on NotifyMyFrontEnd can happen too, and it will also
lead to losing all the other notifications in the local buffer.

> 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:
> do
> {
> ListEntry le;
> qe = (AsyncQueueEntry *) (page_buffer + QUEUE_POS_OFFSET(thisentry));
> reachedEndOfPage = asyncQueueAdvance(current, qe->length);
> ...
> else if (TransactionIdDidCommit(qe->xid))
> {
> le->entry = qe;
> le->nextPos = current;
> }
> } while (!reachedEndOfPage);
>
> foreach(lc, notificationEntries)
> {
> ListEntry *le = lfirst(lc);
> AsyncQueueEntry *qe = le->entry;
> char *channel = qe->data;
>
> PG_TRY()
> {
> if (IsListeningOn(channel))
> {
> char *payload = qe->data + strlen(channel) + 1;
> NotifyMyFrontEnd(channel, payload, qe->srcPid);
> }
> }
> PG_CATCH()
> {
> current = le->nextPos;
> PG_RE_THROW();
> }
> }

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.

- Heikki