> On Sep 21, 2026, at 02:40, Kiran Kaki <[email protected]> wrote: > > Hi Chao > > On Sat, Sep 19, 2026 at 8:08 PM Chao Li <[email protected]> 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/
v2-0001-Fix-inaccurate-LSN-validation-messages-in-pg_wali.patch
Description: Binary data
