Correct comment in StartupXLOG().
Hi,
SharedRecoveryState member of XLogCtl is no longer a boolean flag, got changes
in 4e87c4836ab9 to enum but, comment referring to it still referred as the
boolean flag which is pretty confusing and incorrect.
Also, the last part of the same comment is as:
" .. although the boolean flag to allow WAL is probably atomic in
itself, .....",
I am a bit confused here too about saying "atomic" to it, is that correct?
I haven't done anything about it, only replaced the "boolean flag" to "recovery
state" in the attached patch.
Regards,
Amul
Attachments:
fix_comment_in_StartupXLOG.patchapplication/octet-stream; name=fix_comment_in_StartupXLOG.patchDownload
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index f03bd473e2b..2123e1d2ea8 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -7998,14 +7998,14 @@ StartupXLOG(void)
* All done with end-of-recovery actions.
*
* Now allow backends to write WAL and update the control file status in
- * consequence. The boolean flag allowing backends to write WAL is
+ * consequence. The recovery state allowing backends to write WAL is
* updated while holding ControlFileLock to prevent other backends to look
* at an inconsistent state of the control file in shared memory. There
* is still a small window during which backends can write WAL and the
* control file is still referring to a system not in DB_IN_PRODUCTION
* state while looking at the on-disk control file.
*
- * Also, although the boolean flag to allow WAL is probably atomic in
+ * Also, although the recovery state to allow WAL is probably atomic in
* itself, we use the info_lck here to ensure that there are no race
* conditions concerning visibility of other recent updates to shared
* memory.
On Wed, Feb 3, 2021 at 2:28 PM Amul Sul <sulamul@gmail.com> wrote:
Hi,
SharedRecoveryState member of XLogCtl is no longer a boolean flag, got changes
in 4e87c4836ab9 to enum but, comment referring to it still referred as the
boolean flag which is pretty confusing and incorrect.
+1 for the comment change
Also, the last part of the same comment is as:
" .. although the boolean flag to allow WAL is probably atomic in
itself, .....",I am a bit confused here too about saying "atomic" to it, is that correct?
I haven't done anything about it, only replaced the "boolean flag" to "recovery
state" in the attached patch.
I don't think the atomic is correct, it's no more boolean so it is
better we get rid of this part of the comment
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
On Wed, Feb 3, 2021 at 2:48 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
On Wed, Feb 3, 2021 at 2:28 PM Amul Sul <sulamul@gmail.com> wrote:
Hi,
SharedRecoveryState member of XLogCtl is no longer a boolean flag, got changes
in 4e87c4836ab9 to enum but, comment referring to it still referred as the
boolean flag which is pretty confusing and incorrect.+1 for the comment change
Also, the last part of the same comment is as:
" .. although the boolean flag to allow WAL is probably atomic in
itself, .....",I am a bit confused here too about saying "atomic" to it, is that correct?
I haven't done anything about it, only replaced the "boolean flag" to "recovery
state" in the attached patch.I don't think the atomic is correct, it's no more boolean so it is
better we get rid of this part of the comment
Thanks for the confirmation. Updated that part in the attached version.
Regards,
Amul
Attachments:
v2_fix_comment_in_StartupXLOG.patchapplication/octet-stream; name=v2_fix_comment_in_StartupXLOG.patchDownload
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index f03bd473e2b..75b8609f1b4 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -7998,17 +7998,16 @@ StartupXLOG(void)
* All done with end-of-recovery actions.
*
* Now allow backends to write WAL and update the control file status in
- * consequence. The boolean flag allowing backends to write WAL is
+ * consequence. The recovery state allowing backends to write WAL is
* updated while holding ControlFileLock to prevent other backends to look
* at an inconsistent state of the control file in shared memory. There
* is still a small window during which backends can write WAL and the
* control file is still referring to a system not in DB_IN_PRODUCTION
* state while looking at the on-disk control file.
*
- * Also, although the boolean flag to allow WAL is probably atomic in
- * itself, we use the info_lck here to ensure that there are no race
- * conditions concerning visibility of other recent updates to shared
- * memory.
+ * Also, we use the info_lck to update the recovery state to ensure that
+ * there are no race conditions concerning visibility of other recent
+ * updates to shared memory.
*/
LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
ControlFile->state = DB_IN_PRODUCTION;
At Wed, 3 Feb 2021 16:36:13 +0530, Amul Sul <sulamul@gmail.com> wrote in
On Wed, Feb 3, 2021 at 2:48 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
On Wed, Feb 3, 2021 at 2:28 PM Amul Sul <sulamul@gmail.com> wrote:
Hi,
SharedRecoveryState member of XLogCtl is no longer a boolean flag, got changes
in 4e87c4836ab9 to enum but, comment referring to it still referred as the
boolean flag which is pretty confusing and incorrect.+1 for the comment change
Actually the "flag" has been changed to an integer (emnum), so it
needs a change. However, the current proposal:
* Now allow backends to write WAL and update the control file status in
- * consequence. The boolean flag allowing backends to write WAL is
+ * consequence. The recovery state allowing backends to write WAL is
* updated while holding ControlFileLock to prevent other backends to lookLooks somewhat strange. The old booean had a single task to allow
backends to write WAL but the current state has multple states that
controls recovery progress. So I thnink it needs a further change.
===
Now allow backends to write WAL and update the control file status in
consequence. The recovery state is updated to allow backends to write
WAL, while holding ControlFileLock to prevent other backends to look
at an inconsistent state of the control file in shared memory.
===
Also, the last part of the same comment is as:
" .. although the boolean flag to allow WAL is probably atomic in
itself, .....",I am a bit confused here too about saying "atomic" to it, is that correct?
I haven't done anything about it, only replaced the "boolean flag" to "recovery
state" in the attached patch.I don't think the atomic is correct, it's no more boolean so it is
better we get rid of this part of the commentThanks for the confirmation. Updated that part in the attached version.
I think the original comment still holds except the data type.
- * Also, although the boolean flag to allow WAL is probably atomic in
- * itself, we use the info_lck here to ensure that there are no race
- * conditions concerning visibility of other recent updates to shared
- * memory.
+ * Also, we use the info_lck to update the recovery state to ensure that
+ * there are no race conditions concerning visibility of other recent
+ * updates to shared memory.The type RecoveryState is int, which is of the native machine size
that is considered to be atomic as well as boolean. However, I don't
object to remove the phrase since that removal doesn't change the
point of the description.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
On Thu, Feb 4, 2021 at 6:18 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
At Wed, 3 Feb 2021 16:36:13 +0530, Amul Sul <sulamul@gmail.com> wrote in
On Wed, Feb 3, 2021 at 2:48 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
On Wed, Feb 3, 2021 at 2:28 PM Amul Sul <sulamul@gmail.com> wrote:
Hi,
SharedRecoveryState member of XLogCtl is no longer a boolean flag, got changes
in 4e87c4836ab9 to enum but, comment referring to it still referred as the
boolean flag which is pretty confusing and incorrect.+1 for the comment change
Actually the "flag" has been changed to an integer (emnum), so it
needs a change. However, the current proposal:* Now allow backends to write WAL and update the control file status in - * consequence. The boolean flag allowing backends to write WAL is + * consequence. The recovery state allowing backends to write WAL is * updated while holding ControlFileLock to prevent other backends to lookLooks somewhat strange. The old booean had a single task to allow
backends to write WAL but the current state has multple states that
controls recovery progress. So I thnink it needs a further change.===
Now allow backends to write WAL and update the control file status in
consequence. The recovery state is updated to allow backends to write
WAL, while holding ControlFileLock to prevent other backends to look
at an inconsistent state of the control file in shared memory.
===
This looks more accurate, added the same in the attached version. Thanks for the
correction.
Regards,
Amul
Attachments:
v3_fix_comment_in_StartupXLOG.patchapplication/x-patch; name=v3_fix_comment_in_StartupXLOG.patchDownload
From e72137d632afd4c916da20d56491f782d62b605e Mon Sep 17 00:00:00 2001
From: Amul Sul <amul.sul@enterprisedb.com>
Date: Wed, 3 Feb 2021 23:03:20 -0500
Subject: [PATCH] Correct code comment in StartupXLOG
---
src/backend/access/transam/xlog.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index f03bd473e2b..eee664597ea 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -7998,17 +7998,16 @@ StartupXLOG(void)
* All done with end-of-recovery actions.
*
* Now allow backends to write WAL and update the control file status in
- * consequence. The boolean flag allowing backends to write WAL is
- * updated while holding ControlFileLock to prevent other backends to look
+ * consequence. The recovery state is updated to allow backends to write
+ * WAL while holding ControlFileLock to prevent other backends to look
* at an inconsistent state of the control file in shared memory. There
* is still a small window during which backends can write WAL and the
* control file is still referring to a system not in DB_IN_PRODUCTION
* state while looking at the on-disk control file.
*
- * Also, although the boolean flag to allow WAL is probably atomic in
- * itself, we use the info_lck here to ensure that there are no race
- * conditions concerning visibility of other recent updates to shared
- * memory.
+ * Also, we use the info_lck to update the recovery state to ensure that
+ * there are no race conditions concerning visibility of other recent
+ * updates to shared memory.
*/
LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
ControlFile->state = DB_IN_PRODUCTION;
--
2.18.0
On Thu, Feb 4, 2021 at 9:39 AM Amul Sul <sulamul@gmail.com> wrote:
On Thu, Feb 4, 2021 at 6:18 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:At Wed, 3 Feb 2021 16:36:13 +0530, Amul Sul <sulamul@gmail.com> wrote in
On Wed, Feb 3, 2021 at 2:48 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
On Wed, Feb 3, 2021 at 2:28 PM Amul Sul <sulamul@gmail.com> wrote:
Hi,
SharedRecoveryState member of XLogCtl is no longer a boolean flag, got changes
in 4e87c4836ab9 to enum but, comment referring to it still referred as the
boolean flag which is pretty confusing and incorrect.+1 for the comment change
Actually the "flag" has been changed to an integer (emnum), so it
needs a change. However, the current proposal:* Now allow backends to write WAL and update the control file status in - * consequence. The boolean flag allowing backends to write WAL is + * consequence. The recovery state allowing backends to write WAL is * updated while holding ControlFileLock to prevent other backends to lookLooks somewhat strange. The old booean had a single task to allow
backends to write WAL but the current state has multple states that
controls recovery progress. So I thnink it needs a further change.===
Now allow backends to write WAL and update the control file status in
consequence. The recovery state is updated to allow backends to write
WAL, while holding ControlFileLock to prevent other backends to look
at an inconsistent state of the control file in shared memory.
===This looks more accurate, added the same in the attached version. Thanks for the
correction.
Looks good to me.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
On Thu, Feb 04, 2021 at 12:58:29PM +0530, Dilip Kumar wrote:
Looks good to me.
Rather than using the term "recovery state", I would just use
SharedRecoveryState. This leads me to the attached.
--
Michael
Attachments:
xlog-c-comment.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index f03bd473e2..8e3b5df7dc 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -7998,17 +7998,16 @@ StartupXLOG(void)
* All done with end-of-recovery actions.
*
* Now allow backends to write WAL and update the control file status in
- * consequence. The boolean flag allowing backends to write WAL is
- * updated while holding ControlFileLock to prevent other backends to look
- * at an inconsistent state of the control file in shared memory. There
- * is still a small window during which backends can write WAL and the
- * control file is still referring to a system not in DB_IN_PRODUCTION
+ * consequence. SharedRecoveryState, that controls if backends can write
+ * WAL, is updated while holding ControlFileLock to prevent other backends
+ * to look at an inconsistent state of the control file in shared memory.
+ * There is still a small window during which backends can write WAL and
+ * the control file is still referring to a system not in DB_IN_PRODUCTION
* state while looking at the on-disk control file.
*
- * Also, although the boolean flag to allow WAL is probably atomic in
- * itself, we use the info_lck here to ensure that there are no race
- * conditions concerning visibility of other recent updates to shared
- * memory.
+ * Also, we use info_lck to update SharedRecoveryState to ensure that
+ * there are no race conditions concerning visibility of other recent
+ * updates to shared memory.
*/
LWLockAcquire(ControlFileLock, LW_EXCLUSIVE);
ControlFile->state = DB_IN_PRODUCTION;
On Fri, Feb 5, 2021 at 11:53 AM Michael Paquier <michael@paquier.xyz> wrote:
On Thu, Feb 04, 2021 at 12:58:29PM +0530, Dilip Kumar wrote:
Looks good to me.
Rather than using the term "recovery state", I would just use
SharedRecoveryState. This leads me to the attached.
Alright, that too looks good. Thank you !
Regards,
Amul