Re: pg_walinspect: fix LSN validation messages and empty range handling - Mailing list pgsql-hackers

Hi Chao

On Sat, Sep 19, 2026 at 8:08 PM Chao Li <li.evan.chao@gmail.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.

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
becauseno record was available at or after that LSN. After I ran CHECKPOINT to generate more WAL records, the same
queryreturned 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
WALreader. So that, the record and block information functions can return zero rows, while pg_get_wal_stats() can
preserveits 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."

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

Thanks,
Kiran Kaki.