Regardign RecentFlushPtr in WalSndWaitForWal()
Hi hackers,
I would like to understand why we have code [1]/* Get a more recent flush pointer. */ if (!RecoveryInProgress()) RecentFlushPtr = GetFlushRecPtr(NULL); else RecentFlushPtr = GetXLogReplayRecPtr(NULL); that retrieves
RecentFlushPtr in WalSndWaitForWal() outside of the loop. We utilize
RecentFlushPtr later within the loop, but prior to that, we already
have [2]/* Update our idea of the currently flushed position. */ else if (!RecoveryInProgress()) RecentFlushPtr = GetFlushRecPtr(NULL); else RecentFlushPtr = GetXLogReplayRecPtr(NULL);. Wouldn't [2]/* Update our idea of the currently flushed position. */ else if (!RecoveryInProgress()) RecentFlushPtr = GetFlushRecPtr(NULL); else RecentFlushPtr = GetXLogReplayRecPtr(NULL); alone be sufficient?
Just to check the impact, I ran 'make check-world' after removing [1]/* Get a more recent flush pointer. */ if (!RecoveryInProgress()) RecentFlushPtr = GetFlushRecPtr(NULL); else RecentFlushPtr = GetXLogReplayRecPtr(NULL);,
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);
/* 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);
/* 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]commit 5a991ef8692ed0d170b44958a81a6bd70e90585c Author: Robert Haas <rhaas@postgresql.org> Date: Mon Mar 10 13:50:28 2014 -0400. 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
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
Attachments:
v1-0001-Remove-redundant-RecentFlushPtr-fetch-in-WalSndWa.patchapplication/octet-stream; name=v1-0001-Remove-redundant-RecentFlushPtr-fetch-in-WalSndWa.patchDownload
From ce08367751a08148afa9b8f950a1db5732b1db69 Mon Sep 17 00:00:00 2001
From: Shveta Malik <shveta.malik@gmail.com>
Date: Mon, 11 Mar 2024 15:57:17 +0530
Subject: [PATCH v1] Remove redundant RecentFlushPtr fetch in WalSndWaitForWal
In WalSndWaitForWal(), we fetch RecentFlushPtr both outside the loop
and inside the loop. But we start using RecentFlushPtr only after we
fetch it inside the loop. Thus the one outside the loop can be removed.
---
src/backend/replication/walsender.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 25edb5e141..bc40c454de 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -1831,12 +1831,6 @@ WalSndWaitForWal(XLogRecPtr loc)
!NeedToWaitForWal(loc, RecentFlushPtr, &wait_event))
return RecentFlushPtr;
- /* Get a more recent flush pointer. */
- if (!RecoveryInProgress())
- RecentFlushPtr = GetFlushRecPtr(NULL);
- else
- RecentFlushPtr = GetXLogReplayRecPtr(NULL);
-
/*
* Within the loop, we wait for the necessary WALs to be flushed to disk
* first, followed by waiting for standbys to catch up if there are enough
--
2.34.1
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.