Обсуждение: Regardign RecentFlushPtr in WalSndWaitForWal()

Hi hackers,

I would like to understand why we have code [1] that retrieves
RecentFlushPtr in WalSndWaitForWal() outside of the loop. We utilize
RecentFlushPtr later within the loop, but prior to that, we already
have [2]. Wouldn't [2] alone be sufficient?

Just to check the impact, I ran 'make check-world' after removing [1],
and did not see any issue exposed by the test at-least.

Any thoughts?

[1]:
        /* Get a more recent flush pointer. */
        if (!RecoveryInProgress())
                RecentFlushPtr = GetFlushRecPtr(NULL);
        else
                RecentFlushPtr = GetXLogReplayRecPtr(NULL);

[2]:
                /* Update our idea of the currently flushed position. */
                else if (!RecoveryInProgress())
                        RecentFlushPtr = GetFlushRecPtr(NULL);
                else
                        RecentFlushPtr = GetXLogReplayRecPtr(NULL);

thanks
Shveta

Hi,

On Mon, Feb 26, 2024 at 05:16:39PM +0530, shveta malik wrote:
> Hi hackers,
> 
> I would like to understand why we have code [1] that retrieves
> RecentFlushPtr in WalSndWaitForWal() outside of the loop. We utilize
> RecentFlushPtr later within the loop, but prior to that, we already
> have [2]. Wouldn't [2] alone be sufficient?
> 
> Just to check the impact, I ran 'make check-world' after removing [1],
> and did not see any issue exposed by the test at-least.
> 
> Any thoughts?
> 
> [1]:
>         /* Get a more recent flush pointer. */
>         if (!RecoveryInProgress())
>                 RecentFlushPtr = GetFlushRecPtr(NULL);
>         else
>                 RecentFlushPtr = GetXLogReplayRecPtr(NULL);
> 
> [2]:
>                 /* Update our idea of the currently flushed position. */
>                 else if (!RecoveryInProgress())
>                         RecentFlushPtr = GetFlushRecPtr(NULL);
>                 else
>                         RecentFlushPtr = GetXLogReplayRecPtr(NULL);
> 

It seems to me that [2] alone could be sufficient.

Regards,

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

On Mon, 26 Feb 2024 at 12:46, shveta malik <shveta.malik@gmail.com> wrote:
>
> Hi hackers,
>
> I would like to understand why we have code [1] that retrieves
> RecentFlushPtr in WalSndWaitForWal() outside of the loop. We utilize
> RecentFlushPtr later within the loop, but prior to that, we already
> have [2]. Wouldn't [2] alone be sufficient?
>
> Just to check the impact, I ran 'make check-world' after removing [1],
> and did not see any issue exposed by the test at-least.

Yeah, that seems accurate.

> Any thoughts?
[...]
> [2]:
>                 /* Update our idea of the currently flushed position. */
>                 else if (!RecoveryInProgress())

I can't find where this "else" of this "else if" clause came from, as
this piece of code hasn't changed in years. But apart from that, your
observation seems accurate, yes.

Kind regards,

Matthias van de Meent
Neon (https://neon.tech)

On Fri, Mar 1, 2024 at 4:40 PM Matthias van de Meent
<boekewurm+postgres@gmail.com> wrote:
>
> On Mon, 26 Feb 2024 at 12:46, shveta malik <shveta.malik@gmail.com> wrote:
> >
> > Hi hackers,
> >
> > I would like to understand why we have code [1] that retrieves
> > RecentFlushPtr in WalSndWaitForWal() outside of the loop. We utilize
> > RecentFlushPtr later within the loop, but prior to that, we already
> > have [2]. Wouldn't [2] alone be sufficient?
> >
> > Just to check the impact, I ran 'make check-world' after removing [1],
> > and did not see any issue exposed by the test at-least.
>
> Yeah, that seems accurate.
>
> > Any thoughts?
> [...]
> > [2]:
> >                 /* Update our idea of the currently flushed position. */
> >                 else if (!RecoveryInProgress())
>
> I can't find where this "else" of this "else if" clause came from, as
> this piece of code hasn't changed in years.
>

Right, I think the quoted code has check "if (!RecoveryInProgress())".

>
 But apart from that, your
> observation seems accurate, yes.
>

I also find the observation correct and the code has been like that
since commit 5a991ef8 [1]. So, let's wait to see if Robert or Andres
remembers the reason, otherwise, we should probably nuke this code.


[1]
commit 5a991ef8692ed0d170b44958a81a6bd70e90585c
Author: Robert Haas <rhaas@postgresql.org>
Date:   Mon Mar 10 13:50:28 2014 -0400

    Allow logical decoding via the walsender interface.
--
With Regards,
Amit Kapila.

On Sat, Mar 2, 2024 at 4:44 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> Right, I think the quoted code has check "if (!RecoveryInProgress())".
>
> >
>  But apart from that, your
> > observation seems accurate, yes.
> >
>
> I also find the observation correct and the code has been like that
> since commit 5a991ef8 [1]. So, let's wait to see if Robert or Andres
> remembers the reason, otherwise, we should probably nuke this code.

Please find the patch attached for the same.

thanks
Shveta

On Mon, Mar 11, 2024 at 4:17 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Sat, Mar 2, 2024 at 4:44 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > Right, I think the quoted code has check "if (!RecoveryInProgress())".
> >
> > >
> >  But apart from that, your
> > > observation seems accurate, yes.
> > >
> >
> > I also find the observation correct and the code has been like that
> > since commit 5a991ef8 [1]. So, let's wait to see if Robert or Andres
> > remembers the reason, otherwise, we should probably nuke this code.
>
> Please find the patch attached for the same.
>

LGTM. I'll push this tomorrow unless I see any comments/objections to
this change.

--
With Regards,
Amit Kapila.

On Mon, Mar 11, 2024 at 4:36 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Mar 11, 2024 at 4:17 PM shveta malik <shveta.malik@gmail.com> wrote:
> >
> >
> > Please find the patch attached for the same.
> >
>
> LGTM. I'll push this tomorrow unless I see any comments/objections to
> this change.
>

Pushed.

--
With Regards,
Amit Kapila.