Re: Regardign RecentFlushPtr in WalSndWaitForWal()
On Mon, Mar 11, 2024 at 4:36 PM Amit Kapila wrote: > > On Mon, Mar 11, 2024 at 4:17 PM shveta malik 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.
Re: Regardign RecentFlushPtr in WalSndWaitForWal()
On Mon, Mar 11, 2024 at 4:17 PM shveta malik wrote: > > On Sat, Mar 2, 2024 at 4:44 PM Amit Kapila 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.
Re: Regardign RecentFlushPtr in WalSndWaitForWal()
On Sat, Mar 2, 2024 at 4:44 PM Amit Kapila 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 v1-0001-Remove-redundant-RecentFlushPtr-fetch-in-WalSndWa.patch Description: Binary data
Re: Regardign RecentFlushPtr in WalSndWaitForWal()
On Fri, Mar 1, 2024 at 4:40 PM Matthias van de Meent wrote: > > On Mon, 26 Feb 2024 at 12:46, 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. > > 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 Date: Mon Mar 10 13:50:28 2014 -0400 Allow logical decoding via the walsender interface. -- With Regards, Amit Kapila.
Re: Regardign RecentFlushPtr in WalSndWaitForWal()
On Mon, 26 Feb 2024 at 12:46, 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. 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)
Re: Regardign RecentFlushPtr in WalSndWaitForWal()
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
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
