Re: Correct comment in StartupXLOG().
On Fri, Feb 05, 2021 at 02:42:57PM +0530, Amul Sul wrote: > Alright, that too looks good. Thank you ! Thanks, Amul. I have applied this one. -- Michael signature.asc Description: PGP signature
Re: Correct comment in StartupXLOG().
On Fri, Feb 5, 2021 at 11:53 AM Michael Paquier 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
Re: Correct comment in StartupXLOG().
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 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; signature.asc Description: PGP signature
Re: Correct comment in StartupXLOG().
On Thu, Feb 4, 2021 at 9:39 AM Amul Sul wrote: > > On Thu, Feb 4, 2021 at 6:18 AM Kyotaro Horiguchi > wrote: > > > > At Wed, 3 Feb 2021 16:36:13 +0530, Amul Sul wrote in > > > On Wed, Feb 3, 2021 at 2:48 PM Dilip Kumar wrote: > > > > > > > > On Wed, Feb 3, 2021 at 2:28 PM Amul Sul 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 look > > > > Looks 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
Re: Correct comment in StartupXLOG().
On Thu, Feb 4, 2021 at 6:18 AM Kyotaro Horiguchi wrote: > > At Wed, 3 Feb 2021 16:36:13 +0530, Amul Sul wrote in > > On Wed, Feb 3, 2021 at 2:48 PM Dilip Kumar wrote: > > > > > > On Wed, Feb 3, 2021 at 2:28 PM Amul Sul 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 > look > > Looks 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 From e72137d632afd4c916da20d56491f782d62b605e Mon Sep 17 00:00:00 2001 From: Amul Sul 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
Re: Correct comment in StartupXLOG().
At Wed, 3 Feb 2021 16:36:13 +0530, Amul Sul wrote in > On Wed, Feb 3, 2021 at 2:48 PM Dilip Kumar wrote: > > > > On Wed, Feb 3, 2021 at 2:28 PM Amul Sul 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 look Looks 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 comment > > Thanks 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
Re: Correct comment in StartupXLOG().
On Wed, Feb 3, 2021 at 2:48 PM Dilip Kumar wrote: > > On Wed, Feb 3, 2021 at 2:28 PM Amul Sul 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 v2_fix_comment_in_StartupXLOG.patch Description: Binary data
Re: Correct comment in StartupXLOG().
On Wed, Feb 3, 2021 at 2:28 PM Amul Sul 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
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 fix_comment_in_StartupXLOG.patch Description: Binary data
