Horton

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

Heikki Linnakangas <hlinnaka@iki.fi>

View thread
From: Heikki Linnakangas <hlinnaka@iki.fi>
To: Álvaro Herrera <alvherre@kurilemu.de>
Cc: Joel Jacobson <joel@compiler.org>, Tom Lane <tgl@sss.pgh.pa.us>, pgsql-hackers <pgsql-hackers@postgresql.org>
Date: 2025-11-06T11:35:28Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Clear 'xid' in dummy async notify entries written to fill up pages

  2. Fix remaining race condition with CLOG truncation and LISTEN/NOTIFY

  3. Fix bug where we truncated CLOG that was still needed by LISTEN/NOTIFY

  4. Escalate ERRORs during async notify processing to FATAL

  5. Limit the size of TID lists during parallel GIN build

Thanks for reviewing!

On 06/11/2025 12:24, Álvaro Herrera wrote:
> I've been studying the code for freezing of items at vacuum-truncate
> time, and one thing that jumps at me is that perhaps we ought to be more
> proactive in freezing items immediately as they are processed.  We can
> do that for any transactions that precede RecentXmin, because by that
> point, every snapshot in the system should have that committed
> transaction as no longer in progress anyway. Something like
> 
> diff --git a/src/backend/commands/async.c b/src/backend/commands/async.c
> index 8ac7d989641..88d5ed2b461 100644
> --- a/src/backend/commands/async.c
> +++ b/src/backend/commands/async.c
> @@ -2038,6 +2038,11 @@ asyncQueueProcessPageEntries(volatile QueuePosition *current,
>   			}
>   			else if (TransactionIdDidCommit(qe->xid))
>   			{
> +				if (TransactionIdPrecedes(RecentXmin, qe->xid))
> +				{
> +					elog(WARNING, "freezing an entry for transaction %u", qe->xid);
> +					qe->xid = FrozenTransactionId;
> +				}
>   				memcpy(local_buf_end, qe, qe->length);
>   				local_buf_end += qe->length;
>   			}
> 
> If we do this, then the chances that we need to freeze items at vacuum
> time are much lower, and we're not creating any danger that
> TransactionIdDidCommit() errors any more than we have in the current
> code.
> 
> Is there any reason this wouldn't work?

We could do that and it would be good for performance anyway. This is 
the "hint bit" idea that's been discussed in this thread. I think it 
would be better to do it with additional hint bits rather than by 
overwriting 'xid'. Having a separate COMMIITTED/ABORTED and FROZEN bits 
would allow setting the COMMITTED hint bit even when the xid is newer 
than RecentXmin.

Would need to mark the SLRU page dirty if we do that. Not sure if that 
requires holding an exclusive lock. And we should also replace aborted 
xids with InvalidTransactionId.

I feel that that should be a separate patch, and not backpatched. We'd 
still need all the other changes from this patch series even if we did 
that, it would be just an optimization.

> I noticed that async-notify-test-3 doesn't actually freeze any items by
> itself ... you need to do "ALTER TABLE template0 allow_connections" and
> then do vacuumdb -a in a loop in order for this to happen (or something
> similar, I guess).  Otherwise the new code in AsyncNotifyFreezeXids is
> never executed.

Right, async-notify-test-3 was just a performance test to verify that 
the holding the SLRU lock longer doesn't degrade performance. I haven't 
tried performance testing CLOG truncation itself, but it happens so 
rarely that I'm not too worried about it. See 
src/test/modules/xid_wraparound/t/004_notify_freeze.pl for a correctness 
test.

- Heikki