Re: pg_walinspect: add functions to locate and list WAL by time and LSN - Mailing list pgsql-hackers

> On Sep 30, 2026, at 06:31, surya poondla <suryapoondla4@gmail.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
acceptingend_lsn arguments that 
> are after the server's current LSN", and the next paragraph says FFFFFFFF/FFFFFFFF is equivalent to the current LSN.
Neitherstatement 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
lessthan end LSN", the 
> message the other thread is changing to "less than or equal to", so dropping it avoids two near-identical messages
withdifferent 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
presentin pg_wal". 
> Segment 0 never exists, so this sends the user looking for a retention problem. The other functions reject the same
inputwith "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
anchorwith 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
thestart of the contiguous retained range, and the rescue scan then decodes every segment that remains. So any target
olderthan the retained WAL, such as the clock_timestamp() - interval '100 years' regression test, decodes all of pg_wal
onlyto report "requested time precedes the available WAL range". On a cluster that keeps a lot of WAL, that's a lot of
I/Oto 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_segmentand 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
thathasn't committed 
> yet, for example), the outward scan runs through the whole contiguous range and the function then reports "requested
timeprecedes the 
> available WAL range", even when target_time is well inside retained WAL. If pg_wal also has a gap, it reports a
missingsegment instead, though 
> restoring that segment wouldn't help. This is also the most expensive case, since the whole range has to be decoded
toreach 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
timestampedWAL records are not guaranteed to be ordered monotonically by WAL position. For various reasons, a record
withan 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
willmost likely be an estimate, I think we can tolerate limited timestamp reordering in exchange for decoding
significantlyless 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
thesearch. 

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
specifiedtime range. On successful return, start_timestamp is at or after the lower bound, while end_timestamp is at or
beforethe upper bound. Because the search is approximate, the returned records are not guaranteed to be the globally
closesttimestamped records to those bounds. For example, suppose the requested upper bound is 10:00, and the
timestampedrecords 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
becauseits timestamp is reordered by more than one segment. As mentioned earlier, this degree of timestamp reordering
shouldbe very uncommon on a production cluster. If the user expects the relevant record to be in segment 13, they can
widenthe 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
tothe 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
thisfilter 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
(withand 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
othertimestamped record in the 
> same window, such as a commit from another session under installcheck or from an autovacuum worker, changes the
anchorand 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.cbe 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
filterin 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-basedcandidate 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
earlierbehavior to preserve. 

Agreed.

>
> Finally, pg_get_wal_files() is useful on its own and much closer to ready. Would you consider splitting the two
functionsinto 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/