Обсуждение: Timeline ID hexadecimal format
Hi, I've been puzzled by this message: ~~~ LOG: fetching timeline history file for timeline 17 from primary server FATAL: could not receive timeline history file from the primary server: ERROR: could not open file "pg_xlog/00000011.history": No such file or directory ~~~ It took me a while to understand that the timeline id 11 in hexadecimal is the same as the timeline id 17 in decimal. It appears that the first message is formatted with %u instead of %X, and there some others places with the some format, while WAL filename and history file used hexadecimal. There is another place where timeline id is used : pg_waldump, and in these tools, timeline id ( -t or --timeline ) should be given in decimal, while filename gives it in hexadecimal : imho, it's not user-friendly, and can lead to user's bad input for timeline id. The attached patch proposes to change the format of timelineid from %u to %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? best regards, -- Sébastien
Вложения
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