On Mon, 3 Jun 2024 at 11:21, Peter Smith <smithpb2...@gmail.com> wrote: > > Hi, > > I was looking at this code comment and wondered what it meant. AFAICT > over time code has been moved around causing comments to lose their > original context, so now it is hard to understand what they are > saying. > > ~~~ > > After a 2017 patch [1] the code in walsender.c function > logical_read_xlog_page() looked like this: > > /* make sure we have enough WAL available */ > flushptr = WalSndWaitForWal(targetPagePtr + reqLen); > > /* fail if not (implies we are going to shut down) */ > if (flushptr < targetPagePtr + reqLen) > return -1; > > ~~~ > > The same code in HEAD now looks like this: > > /* > * Make sure we have enough WAL available before retrieving the current > * timeline. This is needed to determine am_cascading_walsender accurately > * which is needed to determine the current timeline. > */ > flushptr = WalSndWaitForWal(targetPagePtr + reqLen); > > /* > * Since logical decoding is also permitted on a standby server, we need > * to check if the server is in recovery to decide how to get the current > * timeline ID (so that it also cover the promotion or timeline change > * cases). > */ > am_cascading_walsender = RecoveryInProgress(); > > if (am_cascading_walsender) > GetXLogReplayRecPtr(&currTLI); > else > currTLI = GetWALInsertionTimeLine(); > > XLogReadDetermineTimeline(state, targetPagePtr, reqLen, currTLI); > sendTimeLineIsHistoric = (state->currTLI != currTLI); > sendTimeLine = state->currTLI; > sendTimeLineValidUpto = state->currTLIValidUntil; > sendTimeLineNextTLI = state->nextTLI; > > /* fail if not (implies we are going to shut down) */ > if (flushptr < targetPagePtr + reqLen) > return -1; > > ~~~ > > Notice how the "fail if not" comment has become distantly separated > from the flushptr assignment it was once adjacent to, so that comment > hardly makes sense anymore -- e.g. "fail if not" WHAT? > > Perhaps the comment should say something like it used to: > /* Fail if there is not enough WAL available. This can happen during > shutdown. */
Agree with this, +1 for this change. Regards, Vignesh
