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

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