Thread: 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
Attachment
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
Attachment
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 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
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 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
Attachment
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 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
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
Attachment
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
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