Hi Nathan, 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.
> On Dec 12, 2025, at 04:28, Nathan Bossart <[email protected]> wrote: > > rebased > > -- > nathan > <v2-0001-Add-percentage-of-transaction-IDs-that-are-availa.patch><v2-0002-Bump-transaction-ID-limit-to-warn-at-100M.patch><v2-0003-Perodically-emit-server-logs-when-fewer-than-500M.patch> 1 - 0001 ``` + (double) (multiWrapLimit - result) / PG_INT32_MAX * 100), ``` I don’t feel good with using PG_INT32_MAX as denominator, though the value is correct. Looking at the code of how xidWrapLimit is calculated: ``` /* * The place where we actually get into deep trouble is halfway around * from the oldest potentially-existing XID. (This calculation is * probably off by one or two counts, because the special XIDs reduce the * size of the loop a little bit. But we throw in plenty of slop below, * so it doesn't matter.) */ xidWrapLimit = oldest_datfrozenxid + (MaxTransactionId >> 1); if (xidWrapLimit < FirstNormalTransactionId) xidWrapLimit += FirstNormalTransactionId; ``` 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. 2 - 0001 ``` + 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%? 3 - 0001 ``` <programlisting> WARNING: database "mydb" must be vacuumed within 39985967 transactions +DETAIL: Approximately 1.86% of transactions IDs are available for use. ``` Typo: " transactions IDs” => " transaction IDs" 4 - 0003 ``` Subject: [PATCH v2 3/3] Perodically emit server logs when fewer than 500M ``` Typo: Perodically => Periodically 5 - 0003 ``` + 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. Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/
