> On Oct 10, 2026, at 07:18, surya poondla <[email protected]> wrote: > > Hi Chao, > > Thanks for v4. The test fix and the doc sentence look good, and CFBot is > green now, including macOS.
Thank you very much for the follow up reviewing. > > I did a full read-through of v4, especially the boundary logic in 0002. > The binary search, the outer anchors, the extra segment scanned on each side, > and the fallback to the current WAL position all look correct to > me. The tie-breaks and the neighbour scan only ever widen the range, which is > the safe direction. Cool! > > I have a few small comments: > 1. pg_get_wal_records_info(start_lsn, end_lsn) includes the start-anchor > record but not the end-anchor one. > end_lsn is the start LSN of the end-anchor record, and > pg_get_wal_records_info() only returns records that end at or before > end_lsn. So if the transaction of interest committed just after the window, > its COMMIT can be the end anchor, the user sees its changes but > not its commit, and might conclude it never committed. I wouldn't change > end_lsn, since pointing pg_get_wal_record_info() at it to see the anchor > is useful, but a sentence in the docs would help. (This only applies when an > end anchor is found, not when end_lsn falls back to the current WAL position.) > This is a good point. Since the requested time range is likely to be only a rough estimate, users may generally be more interested in the surrounding WAL than in the exact endpoint. But I agree that this can sometimes matter. I considered changing end_lsn to the end-anchor record's ending LSN, but then pg_get_wal_record_info(end_lsn) could no longer be used to inspect the end-anchor record directly. Therefore, I added a tip to the doc explaining this situation and how to include the end-anchor record. > 2. The docs say "Unlike the other pg_walinspect functions, this function may > inspect retained WAL from the current timeline's history as well as > the current timeline." I don't think that's accurate. The existing functions > read through read_local_xlog_page_no_wait(), and > XLogReadDetermineTimeline() already follows the timeline history, choosing > each segment's timeline with tliOfPointInHistory(), the same > rule GetWalTimeSegments() uses. I'd drop the sentence. Agreed. > > 3. Nit: "No retained WAL segment contains a timestamped record" is > reported with ERRCODE_INVALID_PARAMETER_VALUE, but nothing is wrong with > the arguments. ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE, which the patch > already uses for "no retained WAL segments are available", seems a better fit. Agreed. > > With these addressed, I think the patch is ready for committer. > > Regards, > Surya Poondla PFA v5: 0001 unchanged; 0002 addressed the new comments. Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/
v5-0001-pg_walinspect-add-function-to-list-WAL-files-by-L.patch
Description: Binary data
v5-0002-pg_walinspect-add-function-to-locate-WAL-by-time.patch
Description: Binary data
