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

On Sat, Jul 6, 2024 at 7:36 PM Bertrand Drouvot
<bertranddrouvot(dot)pg(at)gmail(dot)com> wrote:
>
> On Fri, Jul 05, 2024 at 11:10:00AM +0530, Amit Kapila wrote:
> > On Fri, Jun 28, 2024 at 6:30 PM Bertrand Drouvot
> > <bertranddrouvot(dot)pg(at)gmail(dot)com> wrote:
> > >
> > > On Fri, Jun 28, 2024 at 03:15:22PM +0530, Amit Kapila wrote:
> > > > On Fri, Jun 28, 2024 at 12:55 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> > > > >
> > > >
> > > > I don't know whether your assumption is correct. AFAICS, those two
> > > > lines should be together. Let us ee if Bertrand remembers anything?
> > > >
> > >
> > > IIRC the WalSndWaitForWal() call has been moved to ensure that we can determine
> > > the timeline accurately.
> > >
> >
> > This part is understandable but I don't understand the part of the
> > comment (This is needed to determine am_cascading_walsender accurately
> > ..) atop a call to WalSndWaitForWal(). The am_cascading_walsender is
> > determined based on the results of RecoveryInProgress(). Can the wait
> > for WAL by using WalSndWaitForWal() change the result of
> > RecoveryInProgress()?
>
> No, but WalSndWaitForWal() must be called _before_ assigning
> "am_cascading_walsender = RecoveryInProgress();". The reason is that during
> a promotion am_cascading_walsender must be assigned _after_ the walsender is
> waked up (after the promotion). So that when the walsender exits WalSndWaitForWal(),
> then am_cascading_walsender is assigned "accurately" and so the timeline is.
>
> What I meant to say in this comment is that "am_cascading_walsender = RecoveryInProgress();"
> must be called _after_ "flushptr = WalSndWaitForWal(targetPagePtr + reqLen);".
>
> For example, swaping both lines would cause the 035_standby_logical_decoding.pl
> to fail during the promotion test as the walsender would read from the "previous"
> timeline and then produce things like:
>
> "ERROR: could not find record while sending logically-decoded data: invalid record length at 0/6427B20: expected at least 24, got 0"
>
> To avoid ambiguity should we replace?
>
> "
> /*
> * 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.
> */
> "
>
> With:
>
> "
> /*
> * Make sure we have enough WAL available before retrieving the current
> * timeline. am_cascading_walsender must be assigned after
> * WalSndWaitForWal() (so that it is also correct when the walsender wakes
> * up after a promotion).
> */
> "
>

This sounds better but it is better to add just before we determine
am_cascading_walsender as is done in the attached. What do you think?

BTW, is it possible that the promotion gets completed after we wait
for the required WAL and before assigning am_cascading_walsender? I
think even if that happens we can correctly determine the required
timeline because all the required WAL is already available, is that
correct or am I missing something here?

--
With Regards,
Amit Kapila.