Thread: Re: Inconsistent LSN format in pg_waldump output
On 2025-Jul-01, Japin Li wrote:
> This inconsistency, while minor, could be confusing when cross-referencing
> LSNs within pg_waldump's own output or when parsing it programmatically.
I agree that we should fix this, but I'd rather add the missing zeros
than remove these ones (the only ones we have):
> XLogRecGetLen(record, &rec_len, &fpi_len);
>
> - printf("rmgr: %-11s len (rec/tot): %6u/%6u, tx: %10u, lsn: %X/%08X, prev %X/%08X, ",
> + printf("rmgr: %-11s len (rec/tot): %6u/%6u, tx: %10u, lsn: %X/%X, prev %X/%X, ",
> desc->rm_name,
> rec_len, XLogRecGetTotalLen(record),
> XLogRecGetXid(record),
I think pg_waldump did things right in this regard, and all other places
were cargo-culting the older broken practice.
IOW I think we should change all occurrences of %X/%X to %X/%08X
instead. There's a ton of them though. See also
https://www.postgresql.org/message-id/flat/CAExHW5ub5NaTELZ3hJUCE6amuvqAtsSxc7O%2BuK7y4t9Rrk23cw%40mail.gmail.com
where LSN_FORMAT_ARGS was invented, but where the width of the second %X
was not discussed.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
On Tue, 01 Jul 2025 at 13:39, Álvaro Herrera <alvherre@kurilemu.de> wrote:
> On 2025-Jul-01, Japin Li wrote:
>
>> This inconsistency, while minor, could be confusing when cross-referencing
>> LSNs within pg_waldump's own output or when parsing it programmatically.
>
> I agree that we should fix this, but I'd rather add the missing zeros
> than remove these ones (the only ones we have):
>
>> XLogRecGetLen(record, &rec_len, &fpi_len);
>>
>> - printf("rmgr: %-11s len (rec/tot): %6u/%6u, tx: %10u, lsn: %X/%08X, prev %X/%08X, ",
>> + printf("rmgr: %-11s len (rec/tot): %6u/%6u, tx: %10u, lsn: %X/%X, prev %X/%X, ",
>> desc->rm_name,
>> rec_len, XLogRecGetTotalLen(record),
>> XLogRecGetXid(record),
>
> I think pg_waldump did things right in this regard, and all other places
> were cargo-culting the older broken practice.
>
I initially considered using the %X/%08X format, but observing %X/%X
consistently elsewhere led me to abandon that idea.
> IOW I think we should change all occurrences of %X/%X to %X/%08X
> instead. There's a ton of them though. See also
> https://www.postgresql.org/message-id/flat/CAExHW5ub5NaTELZ3hJUCE6amuvqAtsSxc7O%2BuK7y4t9Rrk23cw%40mail.gmail.com
> where LSN_FORMAT_ARGS was invented, but where the width of the second %X
> was not discussed.
Agreed. I believe %X/%08X is better.
--
Regards,
Japin Li
On Wed, Jul 2, 2025 at 10:56 PM Japin Li <japinli@hotmail.com> wrote:
>
> On Tue, 01 Jul 2025 at 22:00, Japin Li <japinli@hotmail.com> wrote:
> > On Tue, 01 Jul 2025 at 13:39, Álvaro Herrera <alvherre@kurilemu.de> wrote:
> >> On 2025-Jul-01, Japin Li wrote:
> >>
> >>> This inconsistency, while minor, could be confusing when cross-referencing
> >>> LSNs within pg_waldump's own output or when parsing it programmatically.
> >>
> >> I agree that we should fix this, but I'd rather add the missing zeros
> >> than remove these ones (the only ones we have):
> >>
> >>> XLogRecGetLen(record, &rec_len, &fpi_len);
> >>>
> >>> - printf("rmgr: %-11s len (rec/tot): %6u/%6u, tx: %10u, lsn: %X/%08X, prev %X/%08X, ",
> >>> + printf("rmgr: %-11s len (rec/tot): %6u/%6u, tx: %10u, lsn: %X/%X, prev %X/%X, ",
> >>> desc->rm_name,
> >>> rec_len, XLogRecGetTotalLen(record),
> >>> XLogRecGetXid(record),
> >>
> >> I think pg_waldump did things right in this regard, and all other places
> >> were cargo-culting the older broken practice.
> >>
> >
> > I initially considered using the %X/%08X format, but observing %X/%X
> > consistently elsewhere led me to abandon that idea.
> >
> >> IOW I think we should change all occurrences of %X/%X to %X/%08X
> >> instead. There's a ton of them though. See also
> >> https://www.postgresql.org/message-id/flat/CAExHW5ub5NaTELZ3hJUCE6amuvqAtsSxc7O%2BuK7y4t9Rrk23cw%40mail.gmail.com
> >> where LSN_FORMAT_ARGS was invented, but where the width of the second %X
> >> was not discussed.
Interesting. While this is a better format, could it break
compatibility with existing tools that for example compares LSN
strings?
> >
> > Agreed. I believe %X/%08X is better.
>
> Patch to standardize LSN formatting with zero-padding.
Thank you for updating the patch. I think this patch doesn't need to
update .po files as we do that at once when doing the translation
update.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
On 2025-Jul-03, Masahiko Sawada wrote:
> On Wed, Jul 2, 2025 at 10:56 PM Japin Li <japinli@hotmail.com> wrote:
> Interesting. While this is a better format, could it break
> compatibility with existing tools that for example compares LSN
> strings?
I think a tool would have to be severely miswritten in order to become
broken from this change. Our own code to scan LSNs is to use
scanf('%X/%X') which should work just fine with and without the leading
zeroes. I honestly don't see anybody coding this in any different way
that could not cope with the leading zeroes :-)
> > > Agreed. I believe %X/%08X is better.
> >
> > Patch to standardize LSN formatting with zero-padding.
>
> Thank you for updating the patch. I think this patch doesn't need to
> update .po files as we do that at once when doing the translation
> update.
Agreed. In fact these updates are probably harmful (they certainly
bloat the patch quite a bit). These files come from a different
repository, which you're welcome to provide a patch for, after the code
change lands in Postgres.
https://git.postgresql.org/cgit/pgtranslation/messages.git/
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Porque Kim no hacía nada, pero, eso sí,
con extraordinario éxito" ("Kim", Kipling)
On Wed, Jul 02, 2025 at 08:57:45PM +0200, Alvaro Herrera wrote:
> I think a tool would have to be severely miswritten in order to become
> broken from this change. Our own code to scan LSNs is to use
> scanf('%X/%X') which should work just fine with and without the leading
> zeroes. I honestly don't see anybody coding this in any different way
> that could not cope with the leading zeroes :-)
Yep. If you do not want this new policy to be forgotten by new paths,
I'd suggested to standarize that with something like that, close to
the existing LSN_FORMAT_ARGS():
#define LSN_FORMAT "%X/%08X"
I was pretty sure that this point was mentioned when LSN_FORMAT_ARGS
got discussed, but my impression is wrong as Alvaro already said
upthread. I'd suggest to take this extra step now instead of
hardcoding %08X everywhere. We may perhaps go down to backpatch
LSN_FORMAT_ARGS() and this LSN_FORMAT in the back-branches, leading to
less friction when backpatching fixes, but we don't deal with many
stable fixes that would require these.
--
Michael