walsender.c comment with no context is hard to understand
Hi,
I was looking at this code comment and wondered what it meant. AFAICT
over time code has been moved around causing comments to lose their
original context, so now it is hard to understand what they are
saying.
~~~
After a 2017 patch [1]https://github.com/postgres/postgres/commit/fca85f8ef157d4d58dea1fdc8e1f1f957b74ee78 the code in walsender.c function
logical_read_xlog_page() looked like this:
/* make sure we have enough WAL available */
flushptr = WalSndWaitForWal(targetPagePtr + reqLen);
/* fail if not (implies we are going to shut down) */
if (flushptr < targetPagePtr + reqLen)
return -1;
~~~
The same code in HEAD now looks like this:
/*
* 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.
*/
flushptr = WalSndWaitForWal(targetPagePtr + reqLen);
/*
* Since logical decoding is also permitted on a standby server, we need
* to check if the server is in recovery to decide how to get the current
* timeline ID (so that it also cover the promotion or timeline change
* cases).
*/
am_cascading_walsender = RecoveryInProgress();
if (am_cascading_walsender)
GetXLogReplayRecPtr(&currTLI);
else
currTLI = GetWALInsertionTimeLine();
XLogReadDetermineTimeline(state, targetPagePtr, reqLen, currTLI);
sendTimeLineIsHistoric = (state->currTLI != currTLI);
sendTimeLine = state->currTLI;
sendTimeLineValidUpto = state->currTLIValidUntil;
sendTimeLineNextTLI = state->nextTLI;
/* fail if not (implies we are going to shut down) */
if (flushptr < targetPagePtr + reqLen)
return -1;
~~~
Notice how the "fail if not" comment has become distantly separated
from the flushptr assignment it was once adjacent to, so that comment
hardly makes sense anymore -- e.g. "fail if not" WHAT?
Perhaps the comment should say something like it used to:
/* Fail if there is not enough WAL available. This can happen during
shutdown. */
======
[1]: https://github.com/postgres/postgres/commit/fca85f8ef157d4d58dea1fdc8e1f1f957b74ee78
Kind Regards,
Peter Smith.
Fujitsu Australia
On Mon, 3 Jun 2024 at 11:21, Peter Smith <smithpb2250@gmail.com> wrote:
Hi,
I was looking at this code comment and wondered what it meant. AFAICT
over time code has been moved around causing comments to lose their
original context, so now it is hard to understand what they are
saying.~~~
After a 2017 patch [1] the code in walsender.c function
logical_read_xlog_page() looked like this:/* make sure we have enough WAL available */
flushptr = WalSndWaitForWal(targetPagePtr + reqLen);/* fail if not (implies we are going to shut down) */
if (flushptr < targetPagePtr + reqLen)
return -1;~~~
The same code in HEAD now looks like this:
/*
* 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.
*/
flushptr = WalSndWaitForWal(targetPagePtr + reqLen);/*
* Since logical decoding is also permitted on a standby server, we need
* to check if the server is in recovery to decide how to get the current
* timeline ID (so that it also cover the promotion or timeline change
* cases).
*/
am_cascading_walsender = RecoveryInProgress();if (am_cascading_walsender)
GetXLogReplayRecPtr(&currTLI);
else
currTLI = GetWALInsertionTimeLine();XLogReadDetermineTimeline(state, targetPagePtr, reqLen, currTLI);
sendTimeLineIsHistoric = (state->currTLI != currTLI);
sendTimeLine = state->currTLI;
sendTimeLineValidUpto = state->currTLIValidUntil;
sendTimeLineNextTLI = state->nextTLI;/* fail if not (implies we are going to shut down) */
if (flushptr < targetPagePtr + reqLen)
return -1;~~~
Notice how the "fail if not" comment has become distantly separated
from the flushptr assignment it was once adjacent to, so that comment
hardly makes sense anymore -- e.g. "fail if not" WHAT?Perhaps the comment should say something like it used to:
/* Fail if there is not enough WAL available. This can happen during
shutdown. */
Agree with this, +1 for this change.
Regards,
Vignesh
On Wed, Jun 26, 2024 at 02:30:26PM +0530, vignesh C wrote:
On Mon, 3 Jun 2024 at 11:21, Peter Smith <smithpb2250@gmail.com> wrote:
Perhaps the comment should say something like it used to:
/* Fail if there is not enough WAL available. This can happen during
shutdown. */Agree with this, +1 for this change.
That would be an improvement. Would you like to send a patch with all
the areas you think could stand for improvements?
--
Michael
On Thu, Jun 27, 2024 at 3:44 PM Michael Paquier <michael@paquier.xyz> wrote:
On Wed, Jun 26, 2024 at 02:30:26PM +0530, vignesh C wrote:
On Mon, 3 Jun 2024 at 11:21, Peter Smith <smithpb2250@gmail.com> wrote:
Perhaps the comment should say something like it used to:
/* Fail if there is not enough WAL available. This can happen during
shutdown. */Agree with this, +1 for this change.
That would be an improvement. Would you like to send a patch with all
the areas you think could stand for improvements?
--
OK, I attached a patch equivalent of the suggestion in this thread.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
Attachments:
v1-0001-Fix-walsender-comment.patchapplication/octet-stream; name=v1-0001-Fix-walsender-comment.patchDownload
From 2ab833379e5f64ddfa7000f636081477362dad23 Mon Sep 17 00:00:00 2001
From: Peter Smith <peter.b.smith@fujitsu.com>
Date: Fri, 28 Jun 2024 09:39:52 +1000
Subject: [PATCH v1] Fix walsender comment
---
src/backend/replication/walsender.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index c623b07..e0b0dd7 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -1083,7 +1083,10 @@ logical_read_xlog_page(XLogReaderState *state, XLogRecPtr targetPagePtr, int req
sendTimeLineValidUpto = state->currTLIValidUntil;
sendTimeLineNextTLI = state->nextTLI;
- /* fail if not (implies we are going to shut down) */
+ /*
+ * Fail if there is not enough WAL available. This can happen during
+ * shutdown.
+ */
if (flushptr < targetPagePtr + reqLen)
return -1;
--
1.8.3.1
On Fri, Jun 28, 2024 at 5:15 AM Peter Smith <smithpb2250@gmail.com> wrote:
On Thu, Jun 27, 2024 at 3:44 PM Michael Paquier <michael@paquier.xyz> wrote:
On Wed, Jun 26, 2024 at 02:30:26PM +0530, vignesh C wrote:
On Mon, 3 Jun 2024 at 11:21, Peter Smith <smithpb2250@gmail.com> wrote:
Perhaps the comment should say something like it used to:
/* Fail if there is not enough WAL available. This can happen during
shutdown. */Agree with this, +1 for this change.
That would be an improvement. Would you like to send a patch with all
the areas you think could stand for improvements?
--OK, I attached a patch equivalent of the suggestion in this thread.
Shouldn't the check for flushptr (if (flushptr < targetPagePtr +
reqLen)) be moved immediately after the call to WalSndWaitForWal().
The comment seems to suggests the same: "Make sure we have enough WAL
available before retrieving the current timeline. .."
--
With Regards,
Amit Kapila.
On Fri, Jun 28, 2024 at 4:18 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Fri, Jun 28, 2024 at 5:15 AM Peter Smith <smithpb2250@gmail.com> wrote:
On Thu, Jun 27, 2024 at 3:44 PM Michael Paquier <michael@paquier.xyz> wrote:
On Wed, Jun 26, 2024 at 02:30:26PM +0530, vignesh C wrote:
On Mon, 3 Jun 2024 at 11:21, Peter Smith <smithpb2250@gmail.com> wrote:
Perhaps the comment should say something like it used to:
/* Fail if there is not enough WAL available. This can happen during
shutdown. */Agree with this, +1 for this change.
That would be an improvement. Would you like to send a patch with all
the areas you think could stand for improvements?
--OK, I attached a patch equivalent of the suggestion in this thread.
Shouldn't the check for flushptr (if (flushptr < targetPagePtr +
reqLen)) be moved immediately after the call to WalSndWaitForWal().
The comment seems to suggests the same: "Make sure we have enough WAL
available before retrieving the current timeline. .."
Yes, as I wrote in the first post, those lines did once used to be
adjacent in logical_read_xlog_page.
I also wondered if they still belonged together, but I opted for the
safest option of fixing only the comment instead of refactoring old
code when no problem had been reported.
AFAICT these lines became separated due to a 2023 patch [1]https://github.com/postgres/postgres/commit/0fdab27ad68a059a1663fa5ce48d76333f1bd74c#diff-da7052ce339ec037f5c721e08a9532b1fcfb4405966cf6a78d0c811550fd7b43, and you
were one of the reviewers of that patch, so I assumed the code
separation was deemed OK at that time. Unless it was some mistake that
slipped past multiple reviewers?
Kind Regards,
Peter Smith.
Fujitsu Australia
On Fri, Jun 28, 2024 at 12:55 PM Peter Smith <smithpb2250@gmail.com> wrote:
On Fri, Jun 28, 2024 at 4:18 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Fri, Jun 28, 2024 at 5:15 AM Peter Smith <smithpb2250@gmail.com> wrote:
On Thu, Jun 27, 2024 at 3:44 PM Michael Paquier <michael@paquier.xyz> wrote:
On Wed, Jun 26, 2024 at 02:30:26PM +0530, vignesh C wrote:
On Mon, 3 Jun 2024 at 11:21, Peter Smith <smithpb2250@gmail.com> wrote:
Perhaps the comment should say something like it used to:
/* Fail if there is not enough WAL available. This can happen during
shutdown. */Agree with this, +1 for this change.
That would be an improvement. Would you like to send a patch with all
the areas you think could stand for improvements?
--OK, I attached a patch equivalent of the suggestion in this thread.
Shouldn't the check for flushptr (if (flushptr < targetPagePtr +
reqLen)) be moved immediately after the call to WalSndWaitForWal().
The comment seems to suggests the same: "Make sure we have enough WAL
available before retrieving the current timeline. .."Yes, as I wrote in the first post, those lines did once used to be
adjacent in logical_read_xlog_page.I also wondered if they still belonged together, but I opted for the
safest option of fixing only the comment instead of refactoring old
code when no problem had been reported.AFAICT these lines became separated due to a 2023 patch [1], and you
were one of the reviewers of that patch, so I assumed the code
separation was deemed OK at that time. Unless it was some mistake that
slipped past multiple reviewers?
I don't know whether your assumption is correct. AFAICS, those two
lines should be together. Let us ee if Bertrand remembers anything?
--
With Regards,
Amit Kapila.
Hi,
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:
On Fri, Jun 28, 2024 at 4:18 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Fri, Jun 28, 2024 at 5:15 AM Peter Smith <smithpb2250@gmail.com> wrote:
On Thu, Jun 27, 2024 at 3:44 PM Michael Paquier <michael@paquier.xyz> wrote:
On Wed, Jun 26, 2024 at 02:30:26PM +0530, vignesh C wrote:
On Mon, 3 Jun 2024 at 11:21, Peter Smith <smithpb2250@gmail.com> wrote:
Perhaps the comment should say something like it used to:
/* Fail if there is not enough WAL available. This can happen during
shutdown. */Agree with this, +1 for this change.
That would be an improvement. Would you like to send a patch with all
the areas you think could stand for improvements?
--OK, I attached a patch equivalent of the suggestion in this thread.
Shouldn't the check for flushptr (if (flushptr < targetPagePtr +
reqLen)) be moved immediately after the call to WalSndWaitForWal().
The comment seems to suggests the same: "Make sure we have enough WAL
available before retrieving the current timeline. .."Yes, as I wrote in the first post, those lines did once used to be
adjacent in logical_read_xlog_page.I also wondered if they still belonged together, but I opted for the
safest option of fixing only the comment instead of refactoring old
code when no problem had been reported.AFAICT these lines became separated due to a 2023 patch [1], and you
were one of the reviewers of that patch, so I assumed the code
separation was deemed OK at that time. Unless it was some mistake that
slipped past multiple reviewers?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. I agree with Amit that it would make more sense to
move the (flushptr < targetPagePtr + reqLen) check just after the flushptr
assignement. I don't recall that we discussed any reason of not doing so.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
On Fri, Jun 28, 2024 at 4:18 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
...
Shouldn't the check for flushptr (if (flushptr < targetPagePtr +
reqLen)) be moved immediately after the call to WalSndWaitForWal().
The comment seems to suggests the same: "Make sure we have enough WAL
available before retrieving the current timeline. .."
OK, I have changed the code as suggested. Please see the attached v2 patch.
make check-world was successful.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
Attachments:
v2-0001-Relocate-the-flushptr-checking-code.patchapplication/octet-stream; name=v2-0001-Relocate-the-flushptr-checking-code.patchDownload
From 93678f861014c37f36ae178cbc93c51d4387110f Mon Sep 17 00:00:00 2001
From: Peter Smith <peter.b.smith@fujitsu.com>
Date: Mon, 1 Jul 2024 09:23:31 +1000
Subject: [PATCH v2] Relocate the flushptr checking code
---
src/backend/replication/walsender.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index c623b07..12f302c 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -1064,6 +1064,10 @@ logical_read_xlog_page(XLogReaderState *state, XLogRecPtr targetPagePtr, int req
*/
flushptr = WalSndWaitForWal(targetPagePtr + reqLen);
+ /* Fail if not enough (implies we are going to shut down) */
+ if (flushptr < targetPagePtr + reqLen)
+ return -1;
+
/*
* Since logical decoding is also permitted on a standby server, we need
* to check if the server is in recovery to decide how to get the current
@@ -1083,10 +1087,6 @@ logical_read_xlog_page(XLogReaderState *state, XLogRecPtr targetPagePtr, int req
sendTimeLineValidUpto = state->currTLIValidUntil;
sendTimeLineNextTLI = state->nextTLI;
- /* fail if not (implies we are going to shut down) */
- if (flushptr < targetPagePtr + reqLen)
- return -1;
-
if (targetPagePtr + XLOG_BLCKSZ <= flushptr)
count = XLOG_BLCKSZ; /* more than one block available */
else
--
1.8.3.1
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()?
--
With Regards,
Amit Kapila.
On Mon, Jul 1, 2024 at 5:04 AM Peter Smith <smithpb2250@gmail.com> wrote:
On Fri, Jun 28, 2024 at 4:18 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
...
Shouldn't the check for flushptr (if (flushptr < targetPagePtr +
reqLen)) be moved immediately after the call to WalSndWaitForWal().
The comment seems to suggests the same: "Make sure we have enough WAL
available before retrieving the current timeline. .."OK, I have changed the code as suggested. Please see the attached v2 patch.
LGTM. I'll push this early next week unless someone thinks otherwise.
I have added a commit message in the attached.
--
With Regards,
Amit Kapila.
Attachments:
v3-0001-To-improve-the-code-move-the-error-check-in-logic.patchapplication/octet-stream; name=v3-0001-To-improve-the-code-move-the-error-check-in-logic.patchDownload
From 3daa8cb2ec596d5cb528a84a777a6d5ee4581746 Mon Sep 17 00:00:00 2001
From: Amit Kapila <akapila@postgresql.org>
Date: Fri, 5 Jul 2024 12:09:39 +0530
Subject: [PATCH v3] To improve the code, move the error check in
logical_read_xlog_page().
Commit 0fdab27ad6 changed the code to wait for WAL to be available before
determining the timeline but forgot to move the failure check.
This change is to make the related code easier to understand and enhance
otherwise there is no bug in the current code.
Author: Peter Smith
Reviewed-by: Bertrand Drouvot, Amit Kapila
Discussion: https://postgr.es/m/CAHut+PvqX49fusLyXspV1Mmd_EekPtXG0oT146vZjcb9XDvNgw@mail.gmail.com
---
src/backend/replication/walsender.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 754f505c13..e9c88b94c2 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -1062,6 +1062,10 @@ logical_read_xlog_page(XLogReaderState *state, XLogRecPtr targetPagePtr, int req
*/
flushptr = WalSndWaitForWal(targetPagePtr + reqLen);
+ /* Fail if not enough (implies we are going to shut down) */
+ if (flushptr < targetPagePtr + reqLen)
+ return -1;
+
/*
* Since logical decoding is also permitted on a standby server, we need
* to check if the server is in recovery to decide how to get the current
@@ -1081,10 +1085,6 @@ logical_read_xlog_page(XLogReaderState *state, XLogRecPtr targetPagePtr, int req
sendTimeLineValidUpto = state->currTLIValidUntil;
sendTimeLineNextTLI = state->nextTLI;
- /* fail if not (implies we are going to shut down) */
- if (flushptr < targetPagePtr + reqLen)
- return -1;
-
if (targetPagePtr + XLOG_BLCKSZ <= flushptr)
count = XLOG_BLCKSZ; /* more than one block available */
else
--
2.39.1
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 least 24, 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
On Sat, Jul 6, 2024 at 7:36 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
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 least 24, 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).
*/
"
This sounds better but it is better to add just before we determine
am_cascading_walsender as is done in the attached. What do you think?
BTW, is it possible that the promotion gets completed after we wait
for the required WAL and before assigning am_cascading_walsender? I
think even if that happens we can correctly determine the required
timeline because all the required WAL is already available, is that
correct or am I missing something here?
--
With Regards,
Amit Kapila.
Attachments:
v4-0001-To-improve-the-code-move-the-error-check-in-logic.patchapplication/octet-stream; name=v4-0001-To-improve-the-code-move-the-error-check-in-logic.patchDownload
From fa1bdf37d0b1c6c5e2a2f4d38f323f4dd7ca89d1 Mon Sep 17 00:00:00 2001
From: Amit Kapila <akapila@postgresql.org>
Date: Fri, 5 Jul 2024 12:09:39 +0530
Subject: [PATCH v4] To improve the code, move the error check in
logical_read_xlog_page().
Commit 0fdab27ad6 changed the code to wait for WAL to be available before
determining the timeline but forgot to move the failure check.
This change is to make the related code easier to understand and enhance
otherwise there is no bug in the current code.
In the passing, improve the nearby comments to explain why we determine
am_cascading_walsender after waiting for the required WAL.
Author: Peter Smith, Bertrand Drouvot
Reviewed-by: Amit Kapila
Discussion: https://postgr.es/m/CAHut+PvqX49fusLyXspV1Mmd_EekPtXG0oT146vZjcb9XDvNgw@mail.gmail.com
---
src/backend/replication/walsender.c | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 754f505c13..2d1a9ec900 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -1057,16 +1057,21 @@ logical_read_xlog_page(XLogReaderState *state, XLogRecPtr targetPagePtr, int req
/*
* 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.
+ * timeline.
*/
flushptr = WalSndWaitForWal(targetPagePtr + reqLen);
+ /* Fail if not enough (implies we are going to shut down) */
+ if (flushptr < targetPagePtr + reqLen)
+ return -1;
+
/*
* Since logical decoding is also permitted on a standby server, we need
* to check if the server is in recovery to decide how to get the current
- * timeline ID (so that it also cover the promotion or timeline change
- * cases).
+ * timeline ID (so that it also covers the promotion or timeline change
+ * cases). We must determine am_cascading_walsender after waiting for the
+ * required WAL so that it is correct when the walsender wakes up after a
+ * promotion.
*/
am_cascading_walsender = RecoveryInProgress();
@@ -1081,10 +1086,6 @@ logical_read_xlog_page(XLogReaderState *state, XLogRecPtr targetPagePtr, int req
sendTimeLineValidUpto = state->currTLIValidUntil;
sendTimeLineNextTLI = state->nextTLI;
- /* fail if not (implies we are going to shut down) */
- if (flushptr < targetPagePtr + reqLen)
- return -1;
-
if (targetPagePtr + XLOG_BLCKSZ <= flushptr)
count = XLOG_BLCKSZ; /* more than one block available */
else
--
2.28.0.windows.1
Hi,
On Mon, Jul 08, 2024 at 08:46:19AM +0530, Amit Kapila wrote:
This sounds better but it is better to add just before we determine
am_cascading_walsender as is done in the attached. What do you think?
Thanks! LGTM.
BTW, is it possible that the promotion gets completed after we wait
for the required WAL and before assigning am_cascading_walsender?
Yeah, I don't think there is anything that would prevent a promotion to
happen and complete here. I did a few tests (pausing the walsender with gdb at
various places and promoting the standby).
think even if that happens we can correctly determine the required
timeline because all the required WAL is already available, is that
correct
Yeah that's correct.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
On Mon, Jul 8, 2024 at 11:08 AM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
On Mon, Jul 08, 2024 at 08:46:19AM +0530, Amit Kapila wrote:
This sounds better but it is better to add just before we determine
am_cascading_walsender as is done in the attached. What do you think?Thanks! LGTM.
I would like to push this to HEAD only as we don't see any bug that
this change can prevent. What do you think?
--
With Regards,
Amit Kapila.
Hi,
On Mon, Jul 08, 2024 at 11:20:45AM +0530, Amit Kapila wrote:
On Mon, Jul 8, 2024 at 11:08 AM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:On Mon, Jul 08, 2024 at 08:46:19AM +0530, Amit Kapila wrote:
This sounds better but it is better to add just before we determine
am_cascading_walsender as is done in the attached. What do you think?Thanks! LGTM.
I would like to push this to HEAD only as we don't see any bug that
this change can prevent. What do you think?
Yeah, fully agree. I don't see how the previous check location could produce
any bug.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
On Mon, Jul 8, 2024 at 4:19 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
Hi,
On Mon, Jul 08, 2024 at 11:20:45AM +0530, Amit Kapila wrote:
On Mon, Jul 8, 2024 at 11:08 AM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:On Mon, Jul 08, 2024 at 08:46:19AM +0530, Amit Kapila wrote:
This sounds better but it is better to add just before we determine
am_cascading_walsender as is done in the attached. What do you think?Thanks! LGTM.
I would like to push this to HEAD only as we don't see any bug that
this change can prevent. What do you think?Yeah, fully agree. I don't see how the previous check location could produce
any bug.
Hi,
Since the patch was pushed, one recent failure was observed in BF
member rorqual [1]https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=rorqual&dt=2024-07-09%2003%3A46%3A44. but this is probably unrelated to the push because
we found the same failure also occurred in April [2]https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=rorqual&dt=2024-04-18%2006%3A52%3A07&stg=recovery-check.
======
[1]: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=rorqual&dt=2024-07-09%2003%3A46%3A44
[2]: https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=rorqual&dt=2024-04-18%2006%3A52%3A07&stg=recovery-check
Kind Regards,
Peter Smith.
Fujitsu Australia