Re: Timeline ID hexadecimal format
On 21/03/2023 08:15, Peter Eisentraut wrote: On 20.03.23 10:40, Sébastien Lardière wrote: About option_parse_int(), actually, strtoint() is used, do we need a option_parse_ul() fonction ? For the option parsing, I propose the attached patch. This follows the structure of option_parse_int(), so in the future it could be extracted and refactored in the same way, if there is more need. ok for me, it accept 0x values and refuse wrong values committed thanks, -- Sébastien
Re: Timeline ID hexadecimal format
On 20.03.23 10:40, Sébastien Lardière wrote: About option_parse_int(), actually, strtoint() is used, do we need a option_parse_ul() fonction ? For the option parsing, I propose the attached patch. This follows the structure of option_parse_int(), so in the future it could be extracted and refactored in the same way, if there is more need. ok for me, it accept 0x values and refuse wrong values committed
Re: Timeline ID hexadecimal format
On 20/03/2023 09:17, Peter Eisentraut wrote: I have committed the two documentation changes, with some minor adjustments. Thank you, On 07.03.23 18:14, Sébastien Lardière wrote: Maybe this could be fixed instead? Indeed, and strtoul is probably a better option than sscanf, don't you think ? Yeah, the use of sscanf() is kind of weird here. We have been moving the option parsing to use option_parse_int(). Maybe hex support could be added there. Or just use strtoul(). I've made the change with strtoul About option_parse_int(), actually, strtoint() is used, do we need a option_parse_ul() fonction ? For the option parsing, I propose the attached patch. This follows the structure of option_parse_int(), so in the future it could be extracted and refactored in the same way, if there is more need. ok for me, it accept 0x values and refuse wrong values thank you, -- Sébastien
Re: Timeline ID hexadecimal format
I have committed the two documentation changes, with some minor adjustments.
On 07.03.23 18:14, Sébastien Lardière wrote:
Maybe this could be fixed instead?
Indeed, and strtoul is probably a better option than sscanf, don't
you think ?
Yeah, the use of sscanf() is kind of weird here. We have been moving
the option parsing to use option_parse_int(). Maybe hex support could
be added there. Or just use strtoul().
I've made the change with strtoul
About option_parse_int(), actually, strtoint() is used, do we need a
option_parse_ul() fonction ?
For the option parsing, I propose the attached patch. This follows the
structure of option_parse_int(), so in the future it could be extracted
and refactored in the same way, if there is more need.
From a19f9aea4fdeed473d490db6c79a5425bc2bcafd Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Mon, 20 Mar 2023 09:11:35 +0100
Subject: [PATCH v6] pg_waldump: Allow hexadecimal values for -t/--timeline
option
Discussion:
https://www.postgresql.org/message-id/flat/8fef346e-2541-76c3-d768-6536ae052...@lardiere.net
---
doc/src/sgml/ref/pg_waldump.sgml | 3 ++-
src/bin/pg_waldump/pg_waldump.c | 37
2 files changed, 35 insertions(+), 5 deletions(-)
diff --git a/doc/src/sgml/ref/pg_waldump.sgml b/doc/src/sgml/ref/pg_waldump.sgml
index 343f0482a9..7685d3d15b 100644
--- a/doc/src/sgml/ref/pg_waldump.sgml
+++ b/doc/src/sgml/ref/pg_waldump.sgml
@@ -215,7 +215,8 @@ Options
Timeline from which to read WAL records. The default is to use the
value in startseg, if that is specified;
otherwise, the
-default is 1.
+default is 1. The value can be specified in decimal or hexadecimal,
+for example 17 or 0x11.
diff --git a/src/bin/pg_waldump/pg_waldump.c b/src/bin/pg_waldump/pg_waldump.c
index 44b5c8726e..863ef0 100644
--- a/src/bin/pg_waldump/pg_waldump.c
+++ b/src/bin/pg_waldump/pg_waldump.c
@@ -13,6 +13,7 @@
#include "postgres.h"
#include
+#include
#include
#include
#include
@@ -1007,12 +1008,40 @@ main(int argc, char **argv)
private.startptr = (uint64) xlogid <<
32 | xrecoff;
break;
case 't':
- if (sscanf(optarg, "%u", ) !=
1)
+
+ /*
+* This is like option_parse_int() but needs to
handle
+* unsigned 32-bit int. Also, we accept both
decimal and
+* hexadecimal specifications here.
+*/
{
- pg_log_error("invalid timeline
specification: \"%s\"", optarg);
- goto bad_argument;
+ char *endptr;
+ unsigned long val;
+
+ errno = 0;
+ val = strtoul(optarg, , 0);
+
+ while (*endptr != '\0' &&
isspace((unsigned char) *endptr))
+ endptr++;
+
+ if (*endptr != '\0')
+ {
+ pg_log_error("invalid value
\"%s\" for option %s",
+
optarg, "-t/--timeline");
+ goto bad_argument;
+ }
+
+ if (errno == ERANGE || val < 1 || val >
UINT_MAX)
+ {
+ pg_log_error("%s must be in
range %u..%u",
+
"-t/--timeline", 1, UINT_MAX);
+ goto bad_argument;
+ }
+
+ private.timeline = val;
+
+ break;
}
- break;
case 'w':
config.filter_by_fpw = true;
break;
base-commit: 0b51d423e974557e821d890c0a3a49e419a19caa
--
2.39.2
Re: Timeline ID hexadecimal format
On 06/03/2023 18:04, Peter Eisentraut wrote:
On 03.03.23 16:52, Sébastien Lardière wrote:
On 02/03/2023 09:12, Peter Eisentraut wrote:
I think here it would be more helpful to show actual examples. Like,
here is a possible file name, this is what the different parts mean.
So you mean explain the WAL filename and the history filename ? Is it
the good place for it ?
Well, your patch says, by the way, the timeline ID in the file is
hexadecimal. Then one might ask, what file, what is a timeline, what
are the other numbers in the file, etc. It seems very specific in
this context. I don't know if the format of these file names is
actually documented somewhere.
Well, in the context of this patch, the usage both filename are
explained juste before, so it seems understandable to me
Timelines are explained in this place :
https://www.postgresql.org/docs/current/continuous-archiving.html#BACKUP-TIMELINES
so the patch explains the format there
This applies to all configuration parameters, so it doesn't need to
be mentioned explicitly for individual ones.
Probably, but is there another parameter with the same consequence ?
worth it to document this point globally ?
It's ok to mention it again. We do something similar for example at
unix_socket_permissions. But maybe with more context, like "If you
want to specify a timeline ID hexadecimal (for example, if extracted
from a WAL file name), then prefix it with a 0x".
Ok, I've improved the message
Maybe this could be fixed instead?
Indeed, and strtoul is probably a better option than sscanf, don't
you think ?
Yeah, the use of sscanf() is kind of weird here. We have been moving
the option parsing to use option_parse_int(). Maybe hex support could
be added there. Or just use strtoul().
I've made the change with strtoul
About option_parse_int(), actually, strtoint() is used, do we need a
option_parse_ul() fonction ?
patch attached,
best regards,
--
Sébastien
diff --git a/doc/src/sgml/backup.sgml b/doc/src/sgml/backup.sgml
index be05a33205..fb86a3fec5 100644
--- a/doc/src/sgml/backup.sgml
+++ b/doc/src/sgml/backup.sgml
@@ -1332,7 +1332,8 @@ restore_command = 'cp /mnt/server/archivedir/%f %p'
you like, add comments to a history file to record your own notes about
how and why this particular timeline was created. Such comments will be
especially valuable when you have a thicket of different timelines as
-a result of experimentation.
+a result of experimentation. The timeline identifier is an integer which
+is used in hexadecimal format in both WAL segment file names and history files.
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index e5c41cc6c6..6c0d63b73d 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -4110,7 +4110,11 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"' # Windows
current when the base backup was taken. The
value latest recovers
to the latest timeline found in the archive, which is useful in
-a standby server. latest is the default.
+a standby server. A numerical value expressed in hexadecimal, by exemple
+from WAL filename or history file, must be prefixed with 0x,
+for example 0x11 if the WAL filename
+is 001100A1004F.
+latest is the default.
diff --git a/doc/src/sgml/ref/pg_waldump.sgml b/doc/src/sgml/ref/pg_waldump.sgml
index 343f0482a9..d92948c68a 100644
--- a/doc/src/sgml/ref/pg_waldump.sgml
+++ b/doc/src/sgml/ref/pg_waldump.sgml
@@ -215,7 +215,9 @@ PostgreSQL documentation
Timeline from which to read WAL records. The default is to use the
value in startseg, if that is specified; otherwise, the
-default is 1.
+default is 1. The value can be expressed in decimal or hexadecimal format.
+The hexadecimal format, given by WAL filename, must be preceded by 0x,
+by example 0x11.
diff --git a/src/bin/pg_waldump/pg_waldump.c b/src/bin/pg_waldump/pg_waldump.c
index 44b5c8726e..b29d5223ce 100644
--- a/src/bin/pg_waldump/pg_waldump.c
+++ b/src/bin/pg_waldump/pg_waldump.c
@@ -770,6 +770,7 @@ usage(void)
printf(_(" -R, --relation=T/D/R only show records that modify blocks in relation T/D/R\n"));
printf(_(" -s, --start=RECPTR start reading at WAL location RECPTR\n"));
printf(_(" -t, --timeline=TLI timeline from which to read WAL records\n"
+ " hexadecimal value, from WAL filename, must be preceded by 0x\n"
" (default: 1 or the value used in STARTSEG)\n"));
printf(_(" -V, --version output version information, then exit\n"));
printf(_(" -w, --fullpage only show records with a full page write\n"));
@@ -1007,7 +1008,7 @@ main(int argc, char **argv)
private.startptr = (uint64) xlogid << 32 | xrecoff;
break;
Re: Timeline ID hexadecimal format
On 03.03.23 16:52, Sébastien Lardière wrote: On 02/03/2023 09:12, Peter Eisentraut wrote: On 24.02.23 17:27, Sébastien Lardière wrote: diff --git a/doc/src/sgml/backup.sgml b/doc/src/sgml/backup.sgml index be05a33205..7e26b51031 100644 --- a/doc/src/sgml/backup.sgml +++ b/doc/src/sgml/backup.sgml @@ -1332,7 +1332,8 @@ restore_command = 'cp/mnt/server/archivedir/%f %p' you like, add comments to a history file to record your own notes about how and why this particular timeline was created. Such comments will be especially valuable when you have a thicket of different timelines as - a result of experimentation. + a result of experimentation. In both WAL segment file names and history files, + the timeline ID number is expressed in hexadecimal. I think here it would be more helpful to show actual examples. Like, here is a possible file name, this is what the different parts mean. So you mean explain the WAL filename and the history filename ? Is it the good place for it ? Well, your patch says, by the way, the timeline ID in the file is hexadecimal. Then one might ask, what file, what is a timeline, what are the other numbers in the file, etc. It seems very specific in this context. I don't know if the format of these file names is actually documented somewhere. diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index e5c41cc6c6..3b5d041d92 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -4110,7 +4110,9 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"' # Windows current when the base backup was taken. The value latest recovers to the latest timeline found in the archive, which is useful in - a standby server. latest is the default. + a standby server. A numerical value expressed in hexadecimal must be + prefixed with 0x, for example 0x11. + latest is the default. This applies to all configuration parameters, so it doesn't need to be mentioned explicitly for individual ones. Probably, but is there another parameter with the same consequence ? worth it to document this point globally ? It's ok to mention it again. We do something similar for example at unix_socket_permissions. But maybe with more context, like "If you want to specify a timeline ID hexadecimal (for example, if extracted from a WAL file name), then prefix it with a 0x". diff --git a/doc/src/sgml/ref/pg_waldump.sgml b/doc/src/sgml/ref/pg_waldump.sgml index 343f0482a9..4ae8f2ebdd 100644 --- a/doc/src/sgml/ref/pg_waldump.sgml +++ b/doc/src/sgml/ref/pg_waldump.sgml @@ -215,7 +215,8 @@ PostgreSQL documentation Timeline from which to read WAL records. The default is to use the value in startseg, if that is specified; otherwise, the - default is 1. + default is 1. The value must be expressed in decimal, contrary to the hexadecimal + value given in WAL segment file names and history files. Maybe this could be fixed instead? Indeed, and strtoul is probably a better option than sscanf, don't you think ? Yeah, the use of sscanf() is kind of weird here. We have been moving the option parsing to use option_parse_int(). Maybe hex support could be added there. Or just use strtoul().
Re: Timeline ID hexadecimal format
On Tue, Jan 31, 2023 at 2:16 PM Greg Stark wrote: > I don't see any advantage in converting every place where we refer to > timelines into hex and then having to refer to things like timeline > 1A. It doesn't seem any more intuitive to someone understanding what's > going on than referring to timeline 26. The point, though, is that the WAL files we have on disk already say 1A. If we change the log messages to match, that's easier for users. We could alternatively change the naming convention for WAL files on disk, but that feels like a much bigger compatibility break. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Timeline ID hexadecimal format
On 02/03/2023 09:12, Peter Eisentraut wrote: On 24.02.23 17:27, Sébastien Lardière wrote: diff --git a/doc/src/sgml/backup.sgml b/doc/src/sgml/backup.sgml index be05a33205..7e26b51031 100644 --- a/doc/src/sgml/backup.sgml +++ b/doc/src/sgml/backup.sgml @@ -1332,7 +1332,8 @@ restore_command = 'cp/mnt/server/archivedir/%f %p' you like, add comments to a history file to record your own notes about how and why this particular timeline was created. Such comments will be especially valuable when you have a thicket of different timelines as - a result of experimentation. + a result of experimentation. In both WAL segment file names and history files, + the timeline ID number is expressed in hexadecimal. I think here it would be more helpful to show actual examples. Like, here is a possible file name, this is what the different parts mean. So you mean explain the WAL filename and the history filename ? Is it the good place for it ? diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index e5c41cc6c6..3b5d041d92 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -4110,7 +4110,9 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"' # Windows current when the base backup was taken. The value latest recovers to the latest timeline found in the archive, which is useful in - a standby server. latest is the default. + a standby server. A numerical value expressed in hexadecimal must be + prefixed with 0x, for example 0x11. + latest is the default. This applies to all configuration parameters, so it doesn't need to be mentioned explicitly for individual ones. Probably, but is there another parameter with the same consequence ? worth it to document this point globally ? diff --git a/doc/src/sgml/ref/pg_waldump.sgml b/doc/src/sgml/ref/pg_waldump.sgml index 343f0482a9..4ae8f2ebdd 100644 --- a/doc/src/sgml/ref/pg_waldump.sgml +++ b/doc/src/sgml/ref/pg_waldump.sgml @@ -215,7 +215,8 @@ PostgreSQL documentation Timeline from which to read WAL records. The default is to use the value in startseg, if that is specified; otherwise, the - default is 1. + default is 1. The value must be expressed in decimal, contrary to the hexadecimal + value given in WAL segment file names and history files. Maybe this could be fixed instead? Indeed, and strtoul is probably a better option than sscanf, don't you think ? -- Sébastien
Re: Timeline ID hexadecimal format
On 24.02.23 17:27, Sébastien Lardière wrote: diff --git a/doc/src/sgml/backup.sgml b/doc/src/sgml/backup.sgml index be05a33205..7e26b51031 100644 --- a/doc/src/sgml/backup.sgml +++ b/doc/src/sgml/backup.sgml @@ -1332,7 +1332,8 @@ restore_command = 'cp/mnt/server/archivedir/%f %p' you like, add comments to a history file to record your own notes about how and why this particular timeline was created. Such comments will be especially valuable when you have a thicket of different timelines as -a result of experimentation. +a result of experimentation. In both WAL segment file names and history files, +the timeline ID number is expressed in hexadecimal. I think here it would be more helpful to show actual examples. Like, here is a possible file name, this is what the different parts mean. diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index e5c41cc6c6..3b5d041d92 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -4110,7 +4110,9 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"' # Windows current when the base backup was taken. The value latest recovers to the latest timeline found in the archive, which is useful in -a standby server. latest is the default. +a standby server. A numerical value expressed in hexadecimal must be +prefixed with 0x, for example 0x11. +latest is the default. This applies to all configuration parameters, so it doesn't need to be mentioned explicitly for individual ones. diff --git a/doc/src/sgml/ref/pg_waldump.sgml b/doc/src/sgml/ref/pg_waldump.sgml index 343f0482a9..4ae8f2ebdd 100644 --- a/doc/src/sgml/ref/pg_waldump.sgml +++ b/doc/src/sgml/ref/pg_waldump.sgml @@ -215,7 +215,8 @@ PostgreSQL documentation Timeline from which to read WAL records. The default is to use the value in startseg, if that is specified; otherwise, the -default is 1. +default is 1. The value must be expressed in decimal, contrary to the hexadecimal +value given in WAL segment file names and history files. Maybe this could be fixed instead?
Re: Timeline ID hexadecimal format
On 31/01/2023 20:16, Greg Stark wrote: A hint or something just in that case might be enough? It seems to be a -1 ; let's try to improve the documentation, with the attached patch best regards, -- Sébastien diff --git a/doc/src/sgml/backup.sgml b/doc/src/sgml/backup.sgml index be05a33205..7e26b51031 100644 --- a/doc/src/sgml/backup.sgml +++ b/doc/src/sgml/backup.sgml @@ -1332,7 +1332,8 @@ restore_command = 'cp /mnt/server/archivedir/%f %p' you like, add comments to a history file to record your own notes about how and why this particular timeline was created. Such comments will be especially valuable when you have a thicket of different timelines as -a result of experimentation. +a result of experimentation. In both WAL segment file names and history files, +the timeline ID number is expressed in hexadecimal. diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index e5c41cc6c6..3b5d041d92 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -4110,7 +4110,9 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"' # Windows current when the base backup was taken. The value latest recovers to the latest timeline found in the archive, which is useful in -a standby server. latest is the default. +a standby server. A numerical value expressed in hexadecimal must be +prefixed with 0x, for example 0x11. +latest is the default. diff --git a/doc/src/sgml/ref/pg_waldump.sgml b/doc/src/sgml/ref/pg_waldump.sgml index 343f0482a9..4ae8f2ebdd 100644 --- a/doc/src/sgml/ref/pg_waldump.sgml +++ b/doc/src/sgml/ref/pg_waldump.sgml @@ -215,7 +215,8 @@ PostgreSQL documentation Timeline from which to read WAL records. The default is to use the value in startseg, if that is specified; otherwise, the -default is 1. +default is 1. The value must be expressed in decimal, contrary to the hexadecimal +value given in WAL segment file names and history files.
Re: Timeline ID hexadecimal format
On 31/01/2023 20:16, Greg Stark wrote: The fact that the *filename* has it encoded in hex is an implementation detail and really gets exposed here because it's giving you the underlying system error that caused the problem. It's an implementation detail, but an exposed detail, so, people refer to the filename to find the timeline ID (That's why it happened to me) The confusion only arises when the two are juxtaposed. A hint or something just in that case might be enough? Thanks, i got your point. Note that my proposal was to remove the ambiguous notation which happen in some case (as in 11 <-> 17). A hint is useless in most of the case, because there is no ambiguous. That's why i though format hexadecimal everywhere. At least, can I propose to improve the documentation to expose the fact that the timeline ID is exposed in hexadecimal in filenames but must be used in decimal in recovery_target_timeline and pg_waldump ? regards, -- Sébastien
Re: Timeline ID hexadecimal format
I actually find it kind of annoying that we use hex strings for a lot of things where they don't add any value. Namely Transaction ID and LSNs. As a result it's always a bit of a pain to ingest these in other tools or do arithmetic on them. Neither is referring to memory or anything where powers of 2 are significant so it really doesn't buy anything in making them easier to interpret either. I don't see any advantage in converting every place where we refer to timelines into hex and then having to refer to things like timeline 1A. It doesn't seem any more intuitive to someone understanding what's going on than referring to timeline 26. The fact that the *filename* has it encoded in hex is an implementation detail and really gets exposed here because it's giving you the underlying system error that caused the problem. The confusion only arises when the two are juxtaposed. A hint or something just in that case might be enough?
Re: Timeline ID hexadecimal format
On 31/01/2023 10:53, Peter Eisentraut wrote: On 30.01.23 17:05, Sébastien Lardière wrote: Here's the patch with the suggested format ; plus, I add some note in the documentation about recovery_target_timeline, because I don't get how strtoul(), with the special 0 base parameter can work without 0x prefix ; I suppose that nobody use it. I also change pg_controldata and the usage of this output by pg_upgrade. I let internal usages unchanded : content of backup manifest and content of history file. Should I open a commitfest entry, or is it too soon ? It is not too soon. (The next commitfest is open for new patch submissions as soon as the current one is "in progress", which closes it for new patches.) Done : https://commitfest.postgresql.org/42/4155/ -- Sébastien
Re: Timeline ID hexadecimal format
On 31/01/2023 12:26, Ashutosh Bapat wrote:
On Mon, Jan 30, 2023 at 9:35 PM Sébastien Lardière
wrote:
On 27/01/2023 15:55, Peter Eisentraut wrote:
On 27.01.23 14:52, Sébastien Lardière wrote:
The attached patch proposes to change the format of timelineid from
%u to %X.
I think your complaint has merit. But note that if we did a change
like this, then log files or reports from different versions would
have different meaning without a visual difference, which is kind of
what you complained about in the first place. At least we should do
something like 0x%X.
Hi,
Here's the patch with the suggested format ; plus, I add some note in
the documentation about recovery_target_timeline, because I don't get
how strtoul(), with the special 0 base parameter can work without 0x
prefix ; I suppose that nobody use it.
I also change pg_controldata and the usage of this output by pg_upgrade.
I let internal usages unchanded : content of backup manifest and content
of history file.
The patch seems to have some special/unprintable characters in it. I
see a lot ^[[ in there. I can't read the patch because of that.
Sorry for that, it was the --color from git diff, it's fixed, I hope,
thank you
regards,
--
Sébastien
diff --git a/doc/src/sgml/backup.sgml b/doc/src/sgml/backup.sgml
index be05a33205..7e26b51031 100644
--- a/doc/src/sgml/backup.sgml
+++ b/doc/src/sgml/backup.sgml
@@ -1332,7 +1332,8 @@ restore_command = 'cp /mnt/server/archivedir/%f %p'
you like, add comments to a history file to record your own notes about
how and why this particular timeline was created. Such comments will be
especially valuable when you have a thicket of different timelines as
-a result of experimentation.
+a result of experimentation. In both WAL segment file names and history files,
+the timeline ID number is expressed in hexadecimal.
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 1cf53c74ea..508774cfee 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -4110,7 +4110,9 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"' # Windows
current when the base backup was taken. The
value latest recovers
to the latest timeline found in the archive, which is useful in
-a standby server. latest is the default.
+a standby server. A numerical value expressed in hexadecimal must be
+prefixed with 0x, for example 0x11.
+latest is the default.
diff --git a/src/backend/access/rmgrdesc/xlogdesc.c b/src/backend/access/rmgrdesc/xlogdesc.c
index f390c177e4..bdbe993877 100644
--- a/src/backend/access/rmgrdesc/xlogdesc.c
+++ b/src/backend/access/rmgrdesc/xlogdesc.c
@@ -45,7 +45,7 @@ xlog_desc(StringInfo buf, XLogReaderState *record)
CheckPoint *checkpoint = (CheckPoint *) rec;
appendStringInfo(buf, "redo %X/%X; "
- "tli %u; prev tli %u; fpw %s; xid %u:%u; oid %u; multi %u; offset %u; "
+ "tli 0x%X; prev tli 0x%X; fpw %s; xid %u:%u; oid %u; multi %u; offset %u; "
"oldest xid %u in DB %u; oldest multi %u in DB %u; "
"oldest/newest commit timestamp xid: %u/%u; "
"oldest running xid %u; %s",
@@ -135,7 +135,7 @@ xlog_desc(StringInfo buf, XLogReaderState *record)
xl_end_of_recovery xlrec;
memcpy(, rec, sizeof(xl_end_of_recovery));
- appendStringInfo(buf, "tli %u; prev tli %u; time %s",
+ appendStringInfo(buf, "tli 0x%X; prev tli 0x%X; time %s",
xlrec.ThisTimeLineID, xlrec.PrevTimeLineID,
timestamptz_to_str(xlrec.end_time));
}
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index fb4c860bde..c22cf4b2a1 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -7819,7 +7819,7 @@ xlog_redo(XLogReaderState *record)
(void) GetCurrentReplayRecPtr();
if (checkPoint.ThisTimeLineID != replayTLI)
ereport(PANIC,
- (errmsg("unexpected timeline ID %u (should be %u) in shutdown checkpoint record",
+ (errmsg("unexpected timeline ID 0x%X (should be 0x%X) in shutdown checkpoint record",
checkPoint.ThisTimeLineID, replayTLI)));
RecoveryRestartPoint(, record);
@@ -7906,7 +7906,7 @@ xlog_redo(XLogReaderState *record)
(void) GetCurrentReplayRecPtr();
if (xlrec.ThisTimeLineID != replayTLI)
ereport(PANIC,
- (errmsg("unexpected timeline ID %u (should be %u) in end-of-recovery record",
+ (errmsg("unexpected timeline ID 0x%X (should be 0x%X) in end-of-recovery record",
xlrec.ThisTimeLineID, replayTLI)));
}
else if (info == XLOG_NOOP)
diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index aa6c929477..1643d0d98c 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -1329,7 +1329,7 @@ XLogReaderValidatePageHeader(XLogReaderState *state, XLogRecPtr recptr,
XLogFileName(fname, state->seg.ws_tli, segno,
Re: Timeline ID hexadecimal format
On Mon, Jan 30, 2023 at 9:35 PM Sébastien Lardière wrote: > > On 27/01/2023 15:55, Peter Eisentraut wrote: > > On 27.01.23 14:52, Sébastien Lardière wrote: > >> The attached patch proposes to change the format of timelineid from > >> %u to %X. > > > > I think your complaint has merit. But note that if we did a change > > like this, then log files or reports from different versions would > > have different meaning without a visual difference, which is kind of > > what you complained about in the first place. At least we should do > > something like 0x%X. > > > Hi, > > Here's the patch with the suggested format ; plus, I add some note in > the documentation about recovery_target_timeline, because I don't get > how strtoul(), with the special 0 base parameter can work without 0x > prefix ; I suppose that nobody use it. > > I also change pg_controldata and the usage of this output by pg_upgrade. > I let internal usages unchanded : content of backup manifest and content > of history file. The patch seems to have some special/unprintable characters in it. I see a lot ^[[ in there. I can't read the patch because of that. -- Best Wishes, Ashutosh Bapat
Re: Timeline ID hexadecimal format
On 30.01.23 17:05, Sébastien Lardière wrote: Here's the patch with the suggested format ; plus, I add some note in the documentation about recovery_target_timeline, because I don't get how strtoul(), with the special 0 base parameter can work without 0x prefix ; I suppose that nobody use it. I also change pg_controldata and the usage of this output by pg_upgrade. I let internal usages unchanded : content of backup manifest and content of history file. Should I open a commitfest entry, or is it too soon ? It is not too soon. (The next commitfest is open for new patch submissions as soon as the current one is "in progress", which closes it for new patches.)
Re: Timeline ID hexadecimal format
On 27/01/2023 15:55, Peter Eisentraut wrote:
On 27.01.23 14:52, Sébastien Lardière wrote:
The attached patch proposes to change the format of timelineid from
%u to %X.
I think your complaint has merit. But note that if we did a change
like this, then log files or reports from different versions would
have different meaning without a visual difference, which is kind of
what you complained about in the first place. At least we should do
something like 0x%X.
Hi,
Here's the patch with the suggested format ; plus, I add some note in
the documentation about recovery_target_timeline, because I don't get
how strtoul(), with the special 0 base parameter can work without 0x
prefix ; I suppose that nobody use it.
I also change pg_controldata and the usage of this output by pg_upgrade.
I let internal usages unchanded : content of backup manifest and content
of history file.
Should I open a commitfest entry, or is it too soon ?
regards,
--
Sébastien
[1mdiff --git a/doc/src/sgml/backup.sgml b/doc/src/sgml/backup.sgml[m
[1mindex be05a33205..7e26b51031 100644[m
[1m--- a/doc/src/sgml/backup.sgml[m
[1m+++ b/doc/src/sgml/backup.sgml[m
[36m@@ -1332,7 +1332,8 @@[m [mrestore_command = 'cp /mnt/server/archivedir/%f %p'[m
you like, add comments to a history file to record your own notes about[m
how and why this particular timeline was created. Such comments will be[m
especially valuable when you have a thicket of different timelines as[m
[31m-a result of experimentation.[m
[32m+[m[32ma result of experimentation. In both WAL segment file names and history files,[m
[32m+[m[32mthe timeline ID number is expressed in hexadecimal.[m
[m
[m
[m
[1mdiff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml[m
[1mindex 1cf53c74ea..508774cfee 100644[m
[1m--- a/doc/src/sgml/config.sgml[m
[1m+++ b/doc/src/sgml/config.sgml[m
[36m@@ -4110,7 +4110,9 @@[m [mrestore_command = 'copy "C:\\server\\archivedir\\%f" "%p"' # Windows[m
current when the base backup was taken. The[m
value latest recovers[m
to the latest timeline found in the archive, which is useful in[m
[31m-a standby server. latest is the default.[m
[32m+[m[32ma standby server. A numerical value expressed in hexadecimal must be[m
[32m+[m[32mprefixed with 0x, for example 0x11.[m
[32m+[m[32mlatest is the default.[m
[m
[m
[m
[1mdiff --git a/src/backend/access/rmgrdesc/xlogdesc.c b/src/backend/access/rmgrdesc/xlogdesc.c[m
[1mindex f390c177e4..bdbe993877 100644[m
[1m--- a/src/backend/access/rmgrdesc/xlogdesc.c[m
[1m+++ b/src/backend/access/rmgrdesc/xlogdesc.c[m
[36m@@ -45,7 +45,7 @@[m [mxlog_desc(StringInfo buf, XLogReaderState *record)[m
CheckPoint *checkpoint = (CheckPoint *) rec;[m
[m
appendStringInfo(buf, "redo %X/%X; "[m
[31m- "tli %u; prev tli %u; fpw %s; xid %u:%u; oid %u; multi %u; offset %u; "[m
[32m+[m [32m "tli 0x%X; prev tli 0x%X; fpw %s; xid %u:%u; oid %u; multi %u; offset %u; "[m
"oldest xid %u in DB %u; oldest multi %u in DB %u; "[m
"oldest/newest commit timestamp xid: %u/%u; "[m
"oldest running xid %u; %s",[m
[36m@@ -135,7 +135,7 @@[m [mxlog_desc(StringInfo buf, XLogReaderState *record)[m
xl_end_of_recovery xlrec;[m
[m
memcpy(, rec, sizeof(xl_end_of_recovery));[m
[31m- appendStringInfo(buf, "tli %u; prev tli %u; time %s",[m
[32m+[m [32mappendStringInfo(buf, "tli 0x%X; prev tli 0x%X; time %s",[m
xlrec.ThisTimeLineID, xlrec.PrevTimeLineID,[m
timestamptz_to_str(xlrec.end_time));[m
}[m
[1mdiff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c[m
[1mindex fb4c860bde..c22cf4b2a1 100644[m
[1m--- a/src/backend/access/transam/xlog.c[m
[1m+++ b/src/backend/access/transam/xlog.c[m
[36m@@ -7819,7 +7819,7 @@[m [mxlog_redo(XLogReaderState *record)[m
(void) GetCurrentReplayRecPtr();[m
if (checkPoint.ThisTimeLineID != replayTLI)[m
ereport(PANIC,[m
[31m- (errmsg("unexpected timeline ID %u (should be %u) in shutdown checkpoint record",[m
[32m+[m [32m(errmsg("unexpected timeline ID 0x%X (should be 0x%X) in shutdown checkpoint record",[m
checkPoint.ThisTimeLineID, replayTLI)));[m
[m
RecoveryRestartPoint(, record);[m
[36m@@ -7906,7 +7906,7 @@[m [mxlog_redo(XLogReaderState *record)[m
(void) GetCurrentReplayRecPtr();[m
if (xlrec.ThisTimeLineID != replayTLI)[m
ereport(PANIC,[m
[31m- (errmsg("unexpected timeline ID %u (should be %u) in end-of-recovery record",[m
[32m+[m [32m(errmsg("unexpected timeline ID 0x%X (should be 0x%X) in end-of-recovery record",[m
xlrec.ThisTimeLineID, replayTLI)));[m
}[m
else if (info == XLOG_NOOP)[m
[1mdiff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c[m
[1mindex aa6c929477..1643d0d98c
Re: Timeline ID hexadecimal format
On 27/01/2023 15:55, Peter Eisentraut wrote: On 27.01.23 14:52, Sébastien Lardière wrote: The attached patch proposes to change the format of timelineid from %u to %X. I think your complaint has merit. But note that if we did a change like this, then log files or reports from different versions would have different meaning without a visual difference, which is kind of what you complained about in the first place. At least we should do something like 0x%X. Indeed, but the messages that puzzled was in one log file, just together, not in some differents versions. But yes, it should be documented somewhere, actually, I can't find any good place for that, While digging, It seems that recovery_target_timeline should be given in decimal, not in hexadecimal, which seems odd to me ; and pg_controldata use decimal too, not hexadecimal… So, if this idea is correct, the given patch is not enough. Anyway, do you think it is a good idea or not ? Regarding .po files, I don't know how to manage them. Is there any routine to spread the modifications? Or should I identify and change each message? Don't worry about this. This is handled elsewhere. nice, regards, -- Sébastien
Re: Timeline ID hexadecimal format
On 27.01.23 14:52, Sébastien Lardière wrote: The attached patch proposes to change the format of timelineid from %u to %X. I think your complaint has merit. But note that if we did a change like this, then log files or reports from different versions would have different meaning without a visual difference, which is kind of what you complained about in the first place. At least we should do something like 0x%X. Regarding .po files, I don't know how to manage them. Is there any routine to spread the modifications? Or should I identify and change each message? Don't worry about this. This is handled elsewhere.
