Re: BUG #19598: pg_waldump: -s/-e accept out-of-range WAL locations and silently use the low 32 bits

On Wed, Aug 5, 2026, Fujii Masao wrote:
> I think it would be better to improve pg_recvlogical and pg_receivewal
> as well, not just pg_waldump, by introducing a common LSN parsing
> helper in, for example, src/common. That would let frontend tools
> share exactly the same LSN syntax checks.

Agreed -- v2 attached, done that way.

The helper is pg_parse_lsn() in the new src/common/pg_parse_lsn.c,
with the same rules as the backend's pg_lsn_in_safe(): one to eight
hex digits, a slash, one to eight hex digits, and nothing else.
pg_waldump's static helper from v1 moves there, and pg_recvlogical
(-I/-E) and pg_receivewal (-E) now go through it as well.

Three choices worth calling out:

* The helper only parses and returns bool; each tool keeps its own
existing error message ("invalid WAL location" in pg_waldump,
"could not parse start/end position" in the other two), so no error
text changes anywhere. This follows the existing split between
strtoint() in src/common and option_parse_int() in fe_utils.

* No endptr-style variant yet. The three command-line options all
want whole-string parsing. Among the call sites for the separate
patch you describe, pg_rewind's timeline.c and pg_combinebackup's
backup_label.c parse an LSN as a prefix of a longer line, so that
patch will want a second entry point taking an endptr; I did not
add an API with no in-tree caller here. One more data point for
it: parse_manifest.c contains one more sscanf of the same shape,
and it lives in src/common itself, out of reach of fe_utils code
-- which also argues for src/common as the helper's home.

* The backend's pg_lsn_in_safe() is left untouched for now.

The behavior change is confined to the three options: strings the
server rejects as pg_lsn (overlong components, trailing garbage,
leading whitespace, signs, 0x prefixes) now fail with each tool's
existing error instead of running with a mangled location. Strings
the server accepts parse exactly as before; I re-ran the v1 input
matrix against all three tools to check both directions.

The new TAP cases for pg_recvlogical and pg_receivewal fail without
the code change and pass with it; the pg_waldump cases from v1 are
kept. make check-world, a full meson build, and git am on current
master are all clean here.

> BTW, at least for me this looks more like an improvement than a bug fix.
> So I think it should target v20devel.

Makes sense. I'll register the patch in the September commitfest.

I'd appreciate any feedback .

Regards,
Zexin Li

On Wed, Aug 05, 2026 03:03 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:

> On Tue, Aug 4, 2026 at 5:54 PM Zexin Li <lizi(dot)openmind(at)gmail(dot)com> wrote:
> > The same pattern parses user-supplied LSNs in pg_recvlogical (-I/-E)
> > and pg_receivewal (-E); pg_basebackup and pg_rewind only parse
> > server-returned strings. I kept this patch to pg_waldump to match
> > the report's scope, and can send a follow-up moving the helper next
> > to option_parse_int() in fe_utils to cover the other two if that
> > seems worthwhile.
>
> I think it would be better to improve pg_recvlogical and pg_receivewal as
> well,
> not just pg_waldump, by introducing a common LSN parsing helper in,
> for example, src/common. That would let frontend tools share exactly
> the same LSN syntax checks.
>
> pg_basebackup, pg_verifybackup, pg_rewind, and pg_combinebackup also parse
> LSN
> but are a bit different, since they mostly parse LSNs from server
> responses,
> backup manifests, backup_label files, or timeline history files rather
than
> direct command-line input. So they're less likely to see arbitrary invalid
> LSNs
> from users.
>
> Still, if we introduce a common LSN parser, it seems worth considering
> converting those existing sscanf("%X/%08X") call sites as well. That would
> make malformed metadata fail earlier and avoid having several slightly
> different LSN parsers in frontend code. This should be done as a separate
> patch from the pg_waldump/pg_recvlogical/pg_receivewal improvement,
> though.
>
> BTW, at least for me this looks more like an improvement than a bug fix.
> So I think it should target v20devel.
>
> Regards,
>
> --
> Fujii Masao
>

On Wed, Aug 05, 2026 03:03 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:

> On Tue, Aug 4, 2026 at 5:54 PM Zexin Li <lizi(dot)openmind(at)gmail(dot)com> wrote:
> > The same pattern parses user-supplied LSNs in pg_recvlogical (-I/-E)
> > and pg_receivewal (-E); pg_basebackup and pg_rewind only parse
> > server-returned strings. I kept this patch to pg_waldump to match
> > the report's scope, and can send a follow-up moving the helper next
> > to option_parse_int() in fe_utils to cover the other two if that
> > seems worthwhile.
>
> I think it would be better to improve pg_recvlogical and pg_receivewal as
> well,
> not just pg_waldump, by introducing a common LSN parsing helper in,
> for example, src/common. That would let frontend tools share exactly
> the same LSN syntax checks.
>
> pg_basebackup, pg_verifybackup, pg_rewind, and pg_combinebackup also parse
> LSN
> but are a bit different, since they mostly parse LSNs from server
> responses,
> backup manifests, backup_label files, or timeline history files rather than
> direct command-line input. So they're less likely to see arbitrary invalid
> LSNs
> from users.
>
> Still, if we introduce a common LSN parser, it seems worth considering
> converting those existing sscanf("%X/%08X") call sites as well. That would
> make malformed metadata fail earlier and avoid having several slightly
> different LSN parsers in frontend code. This should be done as a separate
> patch from the pg_waldump/pg_recvlogical/pg_receivewal improvement,
> though.
>
> BTW, at least for me this looks more like an improvement than a bug fix.
> So I think it should target v20devel.
>
> Regards,
>
> --
> Fujii Masao
>