Hi hackers, Here're some proposed clean-ups for a few small issues noted while working on [1] [2].
1) Clarify some comments The first patch updates comments in two functions to match the existing code. It separates the reason for rechecking the timeline while waiting for WAL from the .partial rename race. A cascading standby can follow an upstream timeline change while remaining in recovery; the rename race occurs during promotion of the server running the reader. The original comment before invoking XLogReadDetermineTimeline() seems to mix up these two. It also clarifies the read limits(replay for standby and flush for primary), updates currTLIValidUntil to describe the switch LSN itself rather than the rounding boundary, and removes a misleading FIXME: - * Setting tli to our wanted record's TLI is slightly wrong; the - * page might begin on an older timeline if it contains a timeline - * switch, since its xlog segment will have been copied from the - * prior timeline. This is pretty harmless though, as nothing - * cares so long as the timeline doesn't go backwards. We should - * read the page header instead; FIXME someday. This expression seems a bit confusing. In the case of timeline change, the pages before the switch point are copied into the segment file under the name of the new timeline. Those page headers store the old timeline, which should not be used as the tli to open file which may have been renamed or removed. So I think the current behavior is ok -- tli selects the proper segment file to open, which in this case belongs to the new timeline. 2) Remove variable tli and fix a WAL timeline assertion Assert(!XLogRecPtrIsValid(state->currTLIValidUntil) || wantPage + wantLength < state->currTLIValidUntil); We have this assertion after the slow path of reading timeline history. The second part of seems slightly overstrict: wantPage + wantLength < state->currTLIValidUntil wantPage + wantLength points just passed the last byte being read. currTLIValidUntil is exclusive if it's valid. So reading the byte before the currTLIValidUntil seems ok in the case of wantPage + wantLength == state->currTLIValidUntil. We may want to allow the equal case. The patch also removes the variable tli in read_local_xlog_page_guts(), which seems a bit redundant, especially if we remove the FIXME. "Setting tli to our wanted record's TLI is slightly wrong" Instead it pass the selected timeline state->currTLI directly to the WALRead() function. [1] https://www.postgresql.org/message-id/CABPTF7X3JtXNxaQaTu=xnem9qdj46y2xsqq-wptvfk1fqau...@mail.gmail.com [2] https://www.postgresql.org/message-id/CABPTF7W0s_XHGkbsWdVaGs5sBefePXmG64i3RY0FsdU=b_1...@mail.gmail.com -- Regards, Xuneng Zhou HighGo Software Co., Ltd.
v1-0002-Fix-a-WAL-timeline-assertion-and-simplify-timelin.patch
Description: Binary data
v1-0001-Clarify-comments-about-choosing-a-WAL-timeline.patch
Description: Binary data
