Re: walsender.c comment with no context is hard to understand

From: vignesh C <vignesh21(at)gmail(dot)com>
To: Peter Smith <smithpb2250(at)gmail(dot)com>
Cc: PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: walsender.c comment with no context is hard to understand
Date: 2024-06-26 09:00:26
Message-ID: CALDaNm11vn=BK732M2zbguh3We2Zj5s3+N3uxhULA3b+Mq9riQ@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Mon, 3 Jun 2024 at 11:21, Peter Smith <smithpb2250(at)gmail(dot)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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Laurenz Albe 2024-06-26 09:02:30 Re: Wrong security context for deferred triggers?
Previous Message Laurenz Albe 2024-06-26 08:48:03 Re: Proposal: Document ABI Compatibility