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

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.

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.

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.

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.

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

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

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

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?

Regards,
Surya Poondla