Thread: 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
Attachment
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.