Re: pg_resetwal: Corrections around -c option

On 09.10.23 17:48, Alvaro Herrera wrote:
> Hmm, not sure about this.  SLRU_PAGES_PER_SEGMENT is 32, and
> COMMIT_TS_XACTS_PER_PAGE is 819, so this formula gives decimal 26208 =
> 0x6660.  But more generally, I'm not sure about the algorithm.  Really,
> the safe value also depends on how large the latest file actually is;
> e.g., if your numerically greatest file is only 32kB long (four pages)
> then you can't specify values that refer to Xids in pages 5 and beyond,
> because those pages will not have been zeroed into existence yet, so
> you'll get an error:
>    ERROR:  could not access status of transaction 55692
>    DETAIL:  Could not read from file "pg_commit_ts/0002" at offset 32768: read too few bytes.
> I think a useful value can be had by multiplying 26208 by the latest
> *complete* file number, then if there is an incomplete last file, add
> 819 multiplied by the number of pages in it.
> 
> Also, "numerically greatest" is the wrong thing in case you've recently
> wrapped XIDs around but the oldest files (before the wraparound) are
> still present.  You really want the "logically latest" files.  (I think
> that'll coincide with the files having the latest modification times.)

Would those issues also apply to the other SLRU-based guides on this man 
page?  Are they all a bit wrong?

> Not sure how to write this concisely, though.  Should we really try?

Maybe not.  But the documentation currently suggests you can try 
(probably somewhat copy-and-pasted).

>> Second, the present pg_resetwal code hardcodes the minimum value as 2, which
>> is FrozenTransactionId, but it's not clear why that is allowed. Maybe we
>> should change that to FirstNormalTransactionId, which matches other
>> xid-related options in pg_resetwal.
> 
> Yes, that's clearly a mistake I made at the last minute: in [1] I posted
> this patch *without* the test for 2, and when I pushed the patch two
> days later, I had introduced that without any further explanation.
> 
> BTW if you `git show 666e8db` (which is the SHA1 identifier for
> pg_resetxlog.c that appears in the patch I posted back then) you'll see
> that the existing code did not have any similar protection for valid XID
> values.  The tests to FirstNormalTransactionId for -u and -x were
> introduced by commit 74cf7d46a91d, seven years later -- that commit both
> introduced -u as a new feature, and hardened the tests for -x, which was
> previously only testing for zero.

I have committed this.