Re: pg_walinspect: fix LSN validation messages and empty range handling

👍

Kiran reacted via Gmail
<https://www.google.com/gmail/about/?utm_source=gmail-in-product&utm_medium=et&utm_campaign=emojireactionemail#app>

On Sun, Sep 20, 2026, 11:56 PM Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> wrote:

>
>
> > On Sep 21, 2026, at 02:40, Kiran Kaki <itskkpg(at)gmail(dot)com> wrote:
> >
> > Hi Chao
> >
> > On Sat, Sep 19, 2026 at 8:08 PM Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> wrote:
> >>
> >> Hi,
> >>
> >> While working on patch [1], I noticed two small issues with
> pg_walinspect.
> >>
> >> 1. In pg_get_wal_record_info() as well as a few other functions, there
> are checks like:
> >> ```
> >> if (lsn > curr_lsn)
> >> ereport(ERROR,
> >>
> (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> >> errmsg("WAL input LSN must be less than
> current LSN"),
> >> errdetail("Current WAL LSN on the
> database system is at %X/%08X.",
> >>
> LSN_FORMAT_ARGS(curr_lsn))));
> >> ```
> >>
> >> The check itself uses a > comparison, so equality is accepted by this
> validation check. However, the error message says "must be less than",
> which implies that equality is not accepted. Thus, the check and the error
> message are inconsistent.
> >>
> >> Commit 5c1b6628075a changed the check from >= to > and changed the
> error message from "cannot accept future input LSN" to "WAL input LSN must
> be less than current LSN". This seems to have been an oversight.
> >>
> >> The error message can be changed to say "must be less than or equal
> to", matching the actual validation condition.
> >
> > Thanks for the patch. 0001 looks good to me.
>
> Thank you very much for the review.
>
> >
> > As I see 5c1b6628075a relaxed these checks from >= to > but left the
> messages
> > saying "less than", so the wording no longer matches the code. I
> > checked the thread behind that commit to confirm accepting equality
> > was deliberate.
> >
> > Tested on master (9e17d25e79d4):
> >
> > - Applies cleanly, builds with no new warnings
> > - pg_walinspect/regress passes, and so does the full suite
> > - Expected-output changes match the new messages exactly
> > - No other LSN validation message has the same problem
> >
> > Also, the existing tests already cover every message that changed.
> >
> >
> >> 2. pg_get_wal_records_info() accepts an end_lsn equal to start_lsn, but
> the same fixed range can produce different results. For example:
> >> ```
> >> evantest=# select pg_current_wal_flush_lsn();
> >> pg_current_wal_flush_lsn
> >> --------------------------
> >> 0/01D61428
> >> (1 row)
> >> evantest=# SELECT * FROM pg_get_wal_records_info('0/01D61428',
> '0/01D61428');
> >> ERROR: could not find a valid record after 0/01D61428
> >>
> >> evantest=# checkpoint;
> >> CHECKPOINT
> >> evantest=# SELECT * FROM pg_get_wal_records_info('0/01D61428',
> '0/01D61428');
> >> start_lsn | end_lsn | prev_lsn | xid | resource_manager | record_type |
> record_length | main_data_length | fpi_length | description | block_ref
> >>
> -----------+---------+----------+-----+------------------+-------------+---------------+------------------+------------+-------------+-----------
> >> (0 rows)
> >> ```
> >>
> >> When I passed the current flushed LSN to pg_get_wal_records_info() as
> both start_lsn and end_lsn, it raised an error because no record was
> available at or after that LSN. After I ran CHECKPOINT to generate more WAL
> records, the same query returned zero rows.
> >>
> >> Thus, the same fixed range can either raise an error or return zero
> rows depending on whether WAL exists after end_lsn, even though WAL after
> end_lsn cannot belong to the requested range. This may confuse users.
> >>
> >> To fix, I think an empty LSN range cannot contain a complete WAL
> record, so it can be handled without initializing a WAL reader. So that,
> the record and block information functions can return zero rows, while
> pg_get_wal_stats() can preserve its zero-valued aggregate output.
> >>
> >> [1]
> https://www.postgresql.org/message-id/80E9F0AD-CFC5-4BE5-81DE-D8FE35E10A1C%40gmail.com
> >
> > 0002 looks right to me with two suggestions
> >
> > I reproduced the problem on an unpatched build: an empty range at the
> > end of WAL errors with "could not find a valid record after * ",
> > while the same empty range mid-WAL returns 0 rows. So the result
> > depends on whether anything wrote WAL afterwards, which is worth
> > fixing.
> >
> > Tested on master (9e17d25e79d4):
> >
> > - Applies cleanly on top of 0001, builds with no new warnings
> > - pg_walinspect/regress passes, and so does the full suite
> > - Empty ranges now return 0 rows instead of erroring;
> > - Non-empty ranges are unaffected
> >
> > Two small things:
> >
> > 1. Minor: In GetWalStats() the "An empty range cannot contain any WAL
> > records" comment sits above "if (start_lsn < end_lsn)", the
> > opposite sense from the other two sites. The structure has to
> > differ there since it still calls GetXLogSummaryStats(), but the
> > comment reads the wrong way round. Maybe "Read records only if the
> > range is non-empty."
>
> Accepted.
>
> >
> > 2. One thing: pg_get_wal_record_info() isn't covered. Passing it the
> > current LSN still errors in the reader. Same "validation accepts it,
> > then the reader fails" shape, though the right answer is less clear
> > there: it returns a single row rather than a set, so returning nothing
> > isn't an option, and the error isn't inaccurate. I think it would be
> worth
> > handling alongside this, if you agree the shape is the same..
> >
>
> I don’t think we should change pg_get_wal_record_info(). Its doc says:
> ```
> <para>
> If <replaceable>in_lsn</replaceable> isn't at the start of a WAL
> record, information about the next valid WAL record is shown
> instead. If there is no next valid WAL record, the function
> raises an error.
> </para>
> ```
>
> The doc predates 5c1b6628075a, so I don’t think we should change the doc
> behavior.
>
> However, your comment led me to notice that the documentation for
> pg_get_wal_records_info() says:
> ```
> <para>
> The function raises an error if
> <replaceable>start_lsn</replaceable> is not available.
> </para>
> ```
>
> This makes the issue trickier. Before 5c1b6628075a, start_lsn had to be
> less than end_lsn, so an empty range was not accepted and this
> inconsistency could not arise. Based on the current doc, however, it seems
> reasonable for start_lsn == end_lsn to return zero rows when start_lsn is
> available and raise an error when it is not.
>
> From this perspective, I think I should withdraw 0002.
>
> PFA v2:
>
> * 0001 is unchanged
> * Withdraw 0002
>
> Best regards,
> --
> Chao Li (Evan)
> HighGo Software Co., Ltd.
> https://www.highgo.com/
>
>
>
>
>