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

Hi,

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).
*/
"

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com