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.

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

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

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.)

On Mon, Jan 30, 2023 at 9:35 PM Sébastien Lardière
<sebastien@lardiere.net> 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

On 31/01/2023 12:26, Ashutosh Bapat wrote:

On Mon, Jan 30, 2023 at 9:35 PM Sébastien Lardière
<sebastien@lardiere.net> 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

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

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?

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

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

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.
</para>

<para>

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 <literal>latest</literal> recovers
to the latest timeline found in the archive, which is useful in
-        a standby server.  <literal>latest</literal> is the default.
+        a standby server. A numerical value expressed in hexadecimal must be
+        prefixed with <literal>0x</literal>, for example <literal>0x11</literal>.
+        <literal>latest</literal> is the default.
</para>

<para>

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
<para>
Timeline from which to read WAL records. The default is to use the
value in <replaceable>startseg</replaceable>, 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.
</para>
</listitem>
</varlistentry>

Maybe this could be fixed instead?

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.
     </para>
       <para>

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 <literal>latest</literal> recovers
          to the latest timeline found in the archive, which is 
useful in
-        a standby server. <literal>latest</literal> is the default.
+        a standby server. A numerical value expressed in hexadecimal 
must be
+        prefixed with <literal>0x</literal>, for example 
<literal>0x11</literal>.
+        <literal>latest</literal> is the default.
         </para>
           <para>

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
         <para>
          Timeline from which to read WAL records. The default is to 
use the
          value in <replaceable>startseg</replaceable>, 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.
         </para>
        </listitem>
       </varlistentry>

Maybe this could be fixed instead?

Indeed, and strtoul is probably a better option than sscanf, don't you
think ?

--
Sébastien

On Tue, Jan 31, 2023 at 2:16 PM Greg Stark <stark@mit.edu> 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

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.
     </para>
       <para>

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 <literal>latest</literal> recovers
          to the latest timeline found in the archive, which is 
useful in
-        a standby server. <literal>latest</literal> is the default.
+        a standby server. A numerical value expressed in hexadecimal 
must be
+        prefixed with <literal>0x</literal>, for example 
<literal>0x11</literal>.
+        <literal>latest</literal> is the default.
         </para>
           <para>

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
         <para>
          Timeline from which to read WAL records. The default is to 
use the
          value in <replaceable>startseg</replaceable>, 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.
         </para>
        </listitem>
       </varlistentry>

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().

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

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.

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

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

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