Re: walsender.c comment with no context is hard to understand - Mailing list pgsql-hackers

From Bertrand Drouvot
Subject Re: walsender.c comment with no context is hard to understand
Date
Msg-id ZolPW09Z8eRp+8d/@ip-10-97-1-34.eu-west-3.compute.internal
Whole thread Raw
In response to Re: walsender.c comment with no context is hard to understand  (Amit Kapila <amit.kapila16@gmail.com>)
Responses Re: walsender.c comment with no context is hard to understand
List pgsql-hackers
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.pg@gmail.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@gmail.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
least24, 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



pgsql-hackers by date:

Previous
From: "Andrey M. Borodin"
Date:
Subject: Re: Add GiST support for mixed-width integer operators
Next
From: Tom Lane
Date:
Subject: Re: XML test error on Arch Linux