Обсуждение: 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

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

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

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

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