Re: pg_walinspect: add functions to locate and list WAL by time and LSN

> On Sep 30, 2026, at 06:31, surya poondla <suryapoondla4(at)gmail(dot)com> wrote:
>
> Hi Chao,
>
> Thanks for the patch. I think the use case is real. v1 applies cleanly to current master (dca6a9e320e) and builds
> without warnings; the pg_walinspect regression tests pass.

Hi Surya,

Thank you so much for being the first reviewer of this patch.

>
> I have some comments below, starting with pg_get_wal_files(),
>
> Regrading pg_get_wal_files():
> 1. A future end_lsn is rejected, which contradicts what this patch edits.
>
> From the expected output:
>
> SELECT * FROM pg_get_wal_files(pg_current_wal_lsn(), 'FFFFFFFF/FFFFFFFF');
> ERROR: WAL start LSN must be less than end LSN
>
> The patch widens the doc tip to "All of the pg_walinspect functions that accept an LSN range are permissive about accepting end_lsn arguments that
> are after the server's current LSN", and the next paragraph says FFFFFFFF/FFFFFFFF is equivalent to the current LSN. Neither statement holds here.
> The cause is that point_lookup is computed before ValidateInputLSNs() caps end_lsn:
>
> ValidateInputLSNs(start_lsn, &end_lsn);
> point_lookup = (start_lsn == end_lsn);
>
> With that order, the extra "start_lsn >= end_lsn" check can go. It also adds a second copy of "WAL start LSN must be less than end LSN", the
> message the other thread is changing to "less than or equal to", so dropping it avoids two near-identical messages with different meanings.
>

Accepted.

> 2. pg_get_wal_files('0/0', '0/1') reports "WAL segment needed for the requested range is missing" / "Segment 0 is not present in pg_wal".
> Segment 0 never exists, so this sends the user looking for a retention problem. The other functions reject the same input with "could not read WAL
> at LSN 0/00000000", via the lsn < XLOG_BLCKSZ check in InitXLogReaderState(). The same check would fit here.
>

Make sense.

>
> Regarding the pg_get_wal_location_at_time():
>
> 1. If the upper bound is capped at the current time and no timestamped record exists between target_time - before and now, both anchors resolve to the
> same record and the function fails with "could not find a valid WAL range for the requested time window". On a quiet system, asking about something
> that happened 30 seconds ago hits this. "Nothing timestamped since then" isn't really an error. Returning the lower anchor with the current flush
> LSN as the end seems more useful, even though the docs deliberately avoid building an end boundary from the WAL position. What was the reasoning for that?
>
> 2. Regarding the cost, some paths aren't bounded at all. If the lower anchor isn't found, the left scan runs back to the start of the contiguous retained range, and the rescue scan then decodes every segment that remains. So any target older than the retained WAL, such as the clock_timestamp() - interval '100 years' regression test, decodes all of pg_wal only to report "requested time precedes the available WAL range". On a cluster that keeps a lot of WAL, that's a lot of I/O to produce an error. Could the rescue scan be limited, or skipped when the lower anchor can't be found?
>
> Similarly, the "WAL segment needed for the time search is missing" check for a capped upper bound depends only on last_segment and nsegments, which
> are known before any decoding. Moving it ahead of the scan avoids a long scan that is certain to end in that error.
>
> A related case: reading the code, if no retained record carries a timestamp at all (a long-running bulk transaction that hasn't committed
> yet, for example), the outward scan runs through the whole contiguous range and the function then reports "requested time precedes the
> available WAL range", even when target_time is well inside retained WAL. If pg_wal also has a gap, it reports a missing segment instead, though
> restoring that segment wouldn't help. This is also the most expensive case, since the whole range has to be decoded to reach it, so a distinct
> message such as "no retained WAL record contains a timestamp" would at least tell the user what's actually wrong.

I thought more about the design and made some significant changes in v2.

First, I switched to binary search for the lower and upper boundaries. I initially avoided this because timestamps of timestamped WAL records are not guaranteed to be ordered monotonically by WAL position. For various reasons, a record with an earlier timestamp can appear later in WAL, so binary search may return an imprecise result. However, considering that (1) large timestamp reordering should be uncommon on production clusters, and (2) the user-specified time range will most likely be an estimate, I think we can tolerate limited timestamp reordering in exchange for decoding significantly less WAL. The search also examines one additional segment in each direction to accommodate limited reordering. If the returned range does not contain the expected records, the user can widen the time range and repeat the search.

Second, when the search finds only one timestamped WAL record within the specified time range, the function no longer fails. Instead, it returns that record’s LSN as both start_lsn and end_lsn.

Third, to simplify the boundary semantics and implementation, the returned boundary timestamps are now within the specified time range. On successful return, start_timestamp is at or after the lower bound, while end_timestamp is at or before the upper bound. Because the search is approximate, the returned records are not guaranteed to be the globally closest timestamped records to those bounds. For example, suppose the requested upper bound is 10:00, and the timestamped records are:

WAL segment 10: 09:55
WAL segment 11: 10:05
WAL segment 12: 10:06
WAL segment 13: 09:59

The binary search may select segment 10 and examine the adjacent segment 11. It would return 09:55 as the upper boundary. However, the globally closest record at or before 10:00 is the 09:59 record in segment 13, which is missed because its timestamp is reordered by more than one segment. As mentioned earlier, this degree of timestamp reordering should be very uncommon on a production cluster. If the user expects the relevant record to be in segment 13, they can widen the upper boundary, for example to 10:10, and repeat the search.

> 3. Regarding Timelines, GetWalTimeSegments() picks each segment's timeline with tliOfPointInHistory(seg_end, history). Taking the timeline valid at the
> segment's end looks right to me: after a promotion, the new timeline's copy of the switch segment contains all WAL up to the switch point. Without
> archiving, the old timeline's copy isn't renamed to .partial and stays in pg_wal under the same segment number, so this filter is what keeps the two
> apart. However, none of this is exercised. The regression tests run only on timeline 1, and the module has no TAP suite. Given that both the commit
> message and the docs claim ranges can cross a timeline switch, I think this needs a TAP test that promotes a standby (with and without archiving).
>

I was straggling whether or not to add a TAP test and ended up not adding one because the module didn’t have other TAP tests. I can add a TAP test for this timeline switch case.

> 4. The prepared_ok test and the two RESTORE_POINT checks assert that one specific record is the chosen anchor. Any other timestamped record in the
> same window, such as a commit from another session under installcheck or from an autovacuum worker, changes the anchor and fails the test. Checking
> that the record type is one of the timestamp-bearing types would be more robust.

Agreed.

> Also, the four location queries after wal_time_target make the same
> call. The fourth query's assertions imply the first three, and the first differs only in passing the intervals positionally, so the second and third can go.
>

Yep, a bit redundant. Removed the fourth.

> Minor Nits:
> 1) GetXLogRecordTimestamp() only inspects the decoded record, and none of its logic is specific to recovery. Would xlogreader.c be a better home for it than xlogrecovery.c?
> xlogreader.c is also built for frontend programs (pg_waldump compiles it with -DFRONTEND), and the headers it would need, access/xact.h and access/xlog_internal.h, are already included by the
> rmgrdesc code that pg_waldump builds. Exporting it from xlogrecovery.c makes it backend-only, so a later time-based filter in pg_waldump, which today has
> none, couldn't reuse it.

Good point. Accepted.

> 2) The "duplicate WAL segment in current timeline history" ereport looks unreachable, because the tliOfPointInHistory() filter leaves at most one
> file per segment number. It could be an Assert() or elog() rather than a user-facing ERRCODE_DATA_EXCEPTION.

Agreed.

> 3) GetWalTimeSegments(0, &nsegments, NULL) in pg_get_wal_files() passes a target_time that is never used. Moving the mtime-based candidate selection into its own function would remove that argument.

Agreed.

> 4) GetWalTimeSegments() repeats GetCurrentLSN()'s flush/replay LSN logic and adds the insert-timeline selection from read_local_xlog_page_guts(). Giving GetCurrentLSN() an optional TLI out-parameter would keep all three in sync.

Agreed.

> 5) "Preserve the empty-set behavior that STRICT provided for a NULL start": since this is a new function, there's no earlier behavior to preserve.

Agreed.

>
> Finally, pg_get_wal_files() is useful on its own and much closer to ready. Would you consider splitting the two functions into separate patches, so it can move forward while the time-search semantics are worked out?

Good suggestion. I have split the two functions into two commits.

PFA v2.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/