Re: Correct comment in StartupXLOG().

At Wed, 3 Feb 2021 16:36:13 +0530, Amul Sul <sulamul(at)gmail(dot)com> wrote in
> On Wed, Feb 3, 2021 at 2:48 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> >
> > On Wed, Feb 3, 2021 at 2:28 PM Amul Sul <sulamul(at)gmail(dot)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