pg_resetwal: Corrections around -c option
Branching off from [0]/messages/by-id/0f3ab4a1-ae80-56e8-3426-6b4a02507687@eisentraut.org, here is a for-discussion patch about some
corrections for the pg_resetwal -c (--commit-timestamp-ids) option.
First, in the documentation for finding a manual value for the -c option
based on the files present in the data directory, it was missing a
multiplier, like for the other SLRU-based values, and also missing the
mention of adding one for the upper value. The value I came up with is
computed as
SLRU_PAGES_PER_SEGMENT * COMMIT_TS_XACTS_PER_PAGE = 13088 = 0x3320
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.
Thoughts?
[0]: /messages/by-id/0f3ab4a1-ae80-56e8-3426-6b4a02507687@eisentraut.org
/messages/by-id/0f3ab4a1-ae80-56e8-3426-6b4a02507687@eisentraut.org
Attachments:
0001-WIP-pg_resetwal-Corrections-around-c-option.patchtext/plain; charset=UTF-8; name=0001-WIP-pg_resetwal-Corrections-around-c-option.patchDownload
From c092e8767cad0618feee6e6a76071aeb9df3efad Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Thu, 28 Sep 2023 15:43:01 +0200
Subject: [PATCH] WIP: pg_resetwal: Corrections around -c option
In the documentation for finding a manual value for the -c option
based on the files present in the data directory, it was missing a
multiplier, like for the other SLRU-based values, and also missing the
mention of adding one for the upper value. The value mentioned here
is computed as
SLRU_PAGES_PER_SEGMENT * COMMIT_TS_XACTS_PER_PAGE = 13088 = 0x3320
Also, the present pg_resetwal code hardcodes the minimum value as 2,
which is FrozenTransactionId, but it's not clear why that is allowed.
Change that to FirstNormalTransactionId, which matches other
xid-related options in pg_resetwal.
---
doc/src/sgml/ref/pg_resetwal.sgml | 7 ++++---
src/bin/pg_resetwal/pg_resetwal.c | 12 ++++++------
2 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/doc/src/sgml/ref/pg_resetwal.sgml b/doc/src/sgml/ref/pg_resetwal.sgml
index 08cd3ce5fc..40ea1ff724 100644
--- a/doc/src/sgml/ref/pg_resetwal.sgml
+++ b/doc/src/sgml/ref/pg_resetwal.sgml
@@ -183,11 +183,12 @@ <title>Options</title>
A safe value for the oldest transaction ID for which the commit time can
be retrieved (first part) can be determined by looking
for the numerically smallest file name in the directory
- <filename>pg_commit_ts</filename> under the data directory. Conversely, a safe
+ <filename>pg_commit_ts</filename> under the data directory and then
+ multiplying by 13088 (0x3320). Conversely, a safe
value for the newest transaction ID for which the commit time can be
retrieved (second part) can be determined by looking for the numerically
- greatest file name in the same directory. The file names are in
- hexadecimal.
+ greatest file name in the same directory, adding one, and then
+ multiplying by 13088 (0x3320). The file names are in hexadecimal.
</para>
</listitem>
</varlistentry>
diff --git a/src/bin/pg_resetwal/pg_resetwal.c b/src/bin/pg_resetwal/pg_resetwal.c
index 47e05bd2c9..b3fb94dfbe 100644
--- a/src/bin/pg_resetwal/pg_resetwal.c
+++ b/src/bin/pg_resetwal/pg_resetwal.c
@@ -211,13 +211,13 @@ main(int argc, char *argv[])
exit(1);
}
- if (set_oldest_commit_ts_xid < 2 &&
- set_oldest_commit_ts_xid != 0)
- pg_fatal("transaction ID (-c) must be either 0 or greater than or equal to 2");
+ if (set_oldest_commit_ts_xid < FirstNormalTransactionId &&
+ set_oldest_commit_ts_xid != InvalidTransactionId)
+ pg_fatal("transaction ID (-c) must be either %u or greater than or equal to %u", InvalidTransactionId, FirstNormalTransactionId);
- if (set_newest_commit_ts_xid < 2 &&
- set_newest_commit_ts_xid != 0)
- pg_fatal("transaction ID (-c) must be either 0 or greater than or equal to 2");
+ if (set_newest_commit_ts_xid < FirstNormalTransactionId &&
+ set_newest_commit_ts_xid != InvalidTransactionId)
+ pg_fatal("transaction ID (-c) must be either %u or greater than or equal to %u", InvalidTransactionId, FirstNormalTransactionId);
break;
case 'o':
--
2.42.0
On 2023-Sep-28, Peter Eisentraut wrote:
Branching off from [0], here is a for-discussion patch about some
corrections for the pg_resetwal -c (--commit-timestamp-ids) option.First, in the documentation for finding a manual value for the -c option
based on the files present in the data directory, it was missing a
multiplier, like for the other SLRU-based values, and also missing the
mention of adding one for the upper value. The value I came up with is
computed asSLRU_PAGES_PER_SEGMENT * COMMIT_TS_XACTS_PER_PAGE = 13088 = 0x3320
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.)
Not sure how to write this concisely, though. Should we really try?
(I think the number 13088 appeared somewhere in connection with
multixacts. Maybe there was a confusion with that.)
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]/messages/by-id/20141201223413.GH1737@alvh.no-ip.org 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.
[1]: /messages/by-id/20141201223413.GH1737@alvh.no-ip.org
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Las navajas y los monos deben estar siempre distantes" (Germán Poo)
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.
On 2023-Oct-10, Peter Eisentraut wrote:
On 09.10.23 17:48, Alvaro Herrera wrote:
Hmm, not sure about this. [...]
Would those issues also apply to the other SLRU-based guides on this man
page? Are they all a bit wrong?
I didn't verify, but I think it's likely that they do and they are.
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).
I bet this has not been thoroughly verified.
Perhaps we should have (somewhere outside the option list) a separate
paragraph that explains how to determine the safest maximum value to
use, and list only the multiplier together with each option.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/