On Fri, Dec 12, 2025 at 10:59:53AM +0800, Chao Li wrote:
> I just reviewed the patch. My comments are mainly in 0001, and a few nits
> on 0003. For 0002, the code change is quite straightforward, I am not
> sure the value bumping to has been discussed.
Thanks!
> Where "(MaxTransactionId >> 1)” has the same value as PG_INT32_MAX. But
> if one day xid is changed to 64 bits, that code doesn’t need to updated,
> while these patched code will need to be updated.
>
> So, can we define a const in transom.h like:
> ```
> #define MaxTransactionId ((TransactionId) 0xFFFFFFFF)
> #define WrapAroundWindow (MaxTransactionId>>1)
> ```
>
> And use WrapAroundWindow in all places.
I think I'd rather just open-code the (MaxTransactionId / 2) here. I'm not
too concerned about 64-bit transaction IDs (there's a lot more than this to
change for that), but it does seem like a good idea to be consistent with
nearby code.
> ```
> + errdetail("Approximately %.2f%% of MultiXactIds are available for use.",
> ```
>
> “%.2f%%” shows only 2 digits after dot. xidWrapLimit is roughly 2B, when
> remaining goes down to 107374, it will shows “0.00%”. IMO, when remaining
> is a large number, percentage makes more sense, while an exact number is
> clearer when the number is relatively small. So, can we show both
> percentage and exact number? Or shows the exact number when percentage is
> 0.00%?
The errmsg part should already show the exact number of IDs remaining.
> ```
> + xidLogLimit = xidWrapLimit - 500000000;
> ```
>
> Instead of hardcode 500M, do we want to consider
> autovacuum_freeze_max_age? If a deployment sets autovacuum_freeze_max_age
> > 500M, then vacuum would be triggered first, then this log can get kinda
> non-intuitive. But if a vacuum cannot freeze anything tuple, then this
> log will still make sense. I am not sure. Maybe not a real problem.
IMHO we should still emit warnings about imminent wraparound even if
autovacuum_freeze_max_age is set to totally-inadvisable values. I think
the behavior you are describing only happens if users set it to north of
1.6B.
--
nathan
