Re: pg_resetwal: Corrections around -c option

From: Peter Eisentraut <peter(at)eisentraut(dot)org>
To: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
Cc: pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: pg_resetwal: Corrections around -c option
Date: 2023-10-10 07:30:50
Message-ID: 100061e0-efd0-4462-b280-4cbfd6d15b9f@eisentraut.org
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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.

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Amit Kapila 2023-10-10 07:40:38 Re: [PoC] pg_upgrade: allow to upgrade publisher node
Previous Message Drouvot, Bertrand 2023-10-10 07:26:31 Re: Add a new BGWORKER_BYPASS_ROLELOGINCHECK flag