| From: | surya poondla <suryapoondla4(at)gmail(dot)com> |
|---|---|
| To: | Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> |
| Cc: | PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
| Subject: | Re: pg_walinspect: add functions to locate and list WAL by time and LSN |
| Date: | 2026-10-09 23:18:44 |
| Message-ID: | CAOVWO5pXeSQa6ndP0au2rPj3Njx9kL0oZ0Z28a7_eRNeGAQcbQ@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Hi Chao,
Thanks for v4. The test fix and the doc sentence look good, and CFBot is
green now, including macOS.
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.
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.)
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.
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.
With these addressed, I think the patch is ready for committer.
Regards,
Surya Poondla
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Manu | 2026-10-09 23:27:39 | Re: Partition-aware simplification of constant IN lists after partition pruning |
| Previous Message | Haibo Yan | 2026-10-09 22:57:45 | Re: [PATCH v1] Batch B-tree TIDs when building a bitmap |