Обсуждение: Add comments for a postgres program in bootstrap mode

Hi Shiraishi-san,

On Thu, Sep 26, 2019 at 3:06 PM Youki Shiraishi <shiraishi@computer.org> wrote:
>
> Hi,
>
> I have just started to read the PostgreSQL code and found a lack of comments for a postgres backend program in
bootstrapmode.
 
> When I saw the --boot option implemented in src/backend/main/main.c at first time, I did not understand why the
--bootoption is not documented and what it is used for.
 
> The only way to know these things is to type `grep -r '\--boot' .` on a project root.
> It is easy to see that the --boot option is used in initdb for some historical reasons, but it is painful for a
beginnerlike me.
 
> I believe the attached patch which adds a few comments might help a beginner.

Thanks for the patch.  It might be a good idea to demystify this
secret --boot option.

+ /* Bootstrap mode for initdb */
  if (argc > 1 && strcmp(argv[1], "--boot") == 0)
  AuxiliaryProcessMain(argc, argv); /* does not return */
  else if (argc > 1 && strcmp(argv[1], "--describe-config") == 0)

How about expanding that comment just a little bit, say:

    /*
     * Bootstrapping is handled by AuxiliaryProcessMain() for historic
     * reasons.
     */

@@ -190,7 +190,8 @@ static IndexList *ILHead = NULL;
  * AuxiliaryProcessMain
  *
  * The main entry point for auxiliary processes, such as the bgwriter,
- * walwriter, walreceiver, bootstrapper and the shared memory checker code.
+ * walwriter, walreceiver, postgres program in bootstrap mode and the
+ * shared memory checker code.

This change may not be necessary, because, bootstrapper is a good
short name for 'postgres program in bootstrap mode'.  Also, this name
is similar in style to the names of other auxiliary processes.

Thanks,
Amit

Hello Amit,

On Thu, Sep 26, 2019 at 5:38 PM Amit Langote <amitlangote09@gmail.com> wrote:
>
> Hi Shiraishi-san,
>
> On Thu, Sep 26, 2019 at 3:06 PM Youki Shiraishi <shiraishi@computer.org> wrote:
> >
> > Hi,
> >
> > I have just started to read the PostgreSQL code and found a lack of comments for a postgres backend program in
bootstrapmode.
 
> > When I saw the --boot option implemented in src/backend/main/main.c at first time, I did not understand why the
--bootoption is not documented and what it is used for.
 
> > The only way to know these things is to type `grep -r '\--boot' .` on a project root.
> > It is easy to see that the --boot option is used in initdb for some historical reasons, but it is painful for a
beginnerlike me.
 
> > I believe the attached patch which adds a few comments might help a beginner.
>
> Thanks for the patch.  It might be a good idea to demystify this
> secret --boot option.
>
> + /* Bootstrap mode for initdb */
>   if (argc > 1 && strcmp(argv[1], "--boot") == 0)
>   AuxiliaryProcessMain(argc, argv); /* does not return */
>   else if (argc > 1 && strcmp(argv[1], "--describe-config") == 0)
>
> How about expanding that comment just a little bit, say:
>
>     /*
>      * Bootstrapping is handled by AuxiliaryProcessMain() for historic
>      * reasons.
>      */
>
> @@ -190,7 +190,8 @@ static IndexList *ILHead = NULL;
>   * AuxiliaryProcessMain
>   *
>   * The main entry point for auxiliary processes, such as the bgwriter,
> - * walwriter, walreceiver, bootstrapper and the shared memory checker code.
> + * walwriter, walreceiver, postgres program in bootstrap mode and the
> + * shared memory checker code.
>
> This change may not be necessary, because, bootstrapper is a good
> short name for 'postgres program in bootstrap mode'.  Also, this name
> is similar in style to the names of other auxiliary processes.

Thank you for reviewing my patch.
My concern is that the word 'bootstrapper' is ambiguous.
If the word is obvious to hackers, please use the v2 patch attached to
this email.

Thanks,

--
Youki Shiraishi
NTT Software Innovation Center
Phone: +81-(0)3-5860-5115
Email: shiraishi@computer.org

Hi Shiraishi-san,

On Thu, Sep 26, 2019 at 6:32 PM Youki Shiraishi <shiraishi@computer.org> wrote:
> On Thu, Sep 26, 2019 at 5:38 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > On Thu, Sep 26, 2019 at 3:06 PM Youki Shiraishi <shiraishi@computer.org> wrote:
> > > I have just started to read the PostgreSQL code and found a lack of comments for a postgres backend program in
bootstrapmode.
 
> > > When I saw the --boot option implemented in src/backend/main/main.c at first time, I did not understand why the
--bootoption is not documented and what it is used for.
 
> > > The only way to know these things is to type `grep -r '\--boot' .` on a project root.
> > > It is easy to see that the --boot option is used in initdb for some historical reasons, but it is painful for a
beginnerlike me.
 
> > > I believe the attached patch which adds a few comments might help a beginner.
> >
> > Thanks for the patch.  It might be a good idea to demystify this
> > secret --boot option.
> >
> > + /* Bootstrap mode for initdb */
> >   if (argc > 1 && strcmp(argv[1], "--boot") == 0)
> >   AuxiliaryProcessMain(argc, argv); /* does not return */
> >   else if (argc > 1 && strcmp(argv[1], "--describe-config") == 0)
> >
> > How about expanding that comment just a little bit, say:
> >
> >     /*
> >      * Bootstrapping is handled by AuxiliaryProcessMain() for historic
> >      * reasons.
> >      */

Do you any thoughts on this suggestion?

> > @@ -190,7 +190,8 @@ static IndexList *ILHead = NULL;
> >   * AuxiliaryProcessMain
> >   *
> >   * The main entry point for auxiliary processes, such as the bgwriter,
> > - * walwriter, walreceiver, bootstrapper and the shared memory checker code.
> > + * walwriter, walreceiver, postgres program in bootstrap mode and the
> > + * shared memory checker code.
> >
> > This change may not be necessary, because, bootstrapper is a good
> > short name for 'postgres program in bootstrap mode'.  Also, this name
> > is similar in style to the names of other auxiliary processes.
>
> Thank you for reviewing my patch.
> My concern is that the word 'bootstrapper' is ambiguous.

I was saying that 'bootstrapper' sounds like 'bgwriter', 'walwriter',
etc., so fits well in that sentence.  It would've been OK if those
things were also written as 'postgres program that does background
buffer writing', etc.

Thanks,
Amit

Youki Shiraishi <shiraishi@computer.org> writes:
> On Thu, Sep 26, 2019 at 5:38 PM Amit Langote <amitlangote09@gmail.com> wrote:
>>   * The main entry point for auxiliary processes, such as the bgwriter,
>> - * walwriter, walreceiver, bootstrapper and the shared memory checker code.
>> + * walwriter, walreceiver, postgres program in bootstrap mode and the
>> + * shared memory checker code.
>>
>> This change may not be necessary, because, bootstrapper is a good
>> short name for 'postgres program in bootstrap mode'.  Also, this name
>> is similar in style to the names of other auxiliary processes.

> Thank you for reviewing my patch.
> My concern is that the word 'bootstrapper' is ambiguous.
> If the word is obvious to hackers, please use the v2 patch attached to
> this email.

A quick grep through the sources finds that "bootstrapper" is used
in exactly two places (here, and one comment in initdb.c).  I don't
think it's accepted jargon at all, and would vote to get rid of it.
What I think *is* the usual phrasing is "bootstrap-mode backend"
or variants of that.

            regards, tom lane

Hello Tom,

On Fri, Sep 27, 2019 at 12:23 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Youki Shiraishi <shiraishi@computer.org> writes:
> > On Thu, Sep 26, 2019 at 5:38 PM Amit Langote <amitlangote09@gmail.com> wrote:
> >>   * The main entry point for auxiliary processes, such as the bgwriter,
> >> - * walwriter, walreceiver, bootstrapper and the shared memory checker code.
> >> + * walwriter, walreceiver, postgres program in bootstrap mode and the
> >> + * shared memory checker code.
> >>
> >> This change may not be necessary, because, bootstrapper is a good
> >> short name for 'postgres program in bootstrap mode'.  Also, this name
> >> is similar in style to the names of other auxiliary processes.
>
> > Thank you for reviewing my patch.
> > My concern is that the word 'bootstrapper' is ambiguous.
> > If the word is obvious to hackers, please use the v2 patch attached to
> > this email.
>
> A quick grep through the sources finds that "bootstrapper" is used
> in exactly two places (here, and one comment in initdb.c).  I don't
> think it's accepted jargon at all, and would vote to get rid of it.
> What I think *is* the usual phrasing is "bootstrap-mode backend"
> or variants of that.

I also vote to get rid of such ambiguous stuff.
As you can see by grepping, "bootstrap-mode backend" (and something
like that) is also called in the sources as:

- bootstrap backend
- (basic) bootstrap process
- backend running in bootstrap mode
- postgres (backend) program in bootstrap mode
- bootstrapper

I think "bootstrap backend" is a strong candidate for an alternative
of "bootstrapper" because it is used in the official documentation of
initdb.
--
Youki Shiraishi
NTT Software Innovation Center
Phone: +81-(0)3-5860-5115
Email: shiraishi@computer.org

Hi Amit,

On Fri, Sep 27, 2019 at 12:10 AM Amit Langote <amitlangote09@gmail.com> wrote:
>
> Hi Shiraishi-san,
>
> On Thu, Sep 26, 2019 at 6:32 PM Youki Shiraishi <shiraishi@computer.org> wrote:
> > On Thu, Sep 26, 2019 at 5:38 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > > On Thu, Sep 26, 2019 at 3:06 PM Youki Shiraishi <shiraishi@computer.org> wrote:
> > > > I have just started to read the PostgreSQL code and found a lack of comments for a postgres backend program in
bootstrapmode.
 
> > > > When I saw the --boot option implemented in src/backend/main/main.c at first time, I did not understand why the
--bootoption is not documented and what it is used for.
 
> > > > The only way to know these things is to type `grep -r '\--boot' .` on a project root.
> > > > It is easy to see that the --boot option is used in initdb for some historical reasons, but it is painful for a
beginnerlike me.
 
> > > > I believe the attached patch which adds a few comments might help a beginner.
> > >
> > > Thanks for the patch.  It might be a good idea to demystify this
> > > secret --boot option.
> > >
> > > + /* Bootstrap mode for initdb */
> > >   if (argc > 1 && strcmp(argv[1], "--boot") == 0)
> > >   AuxiliaryProcessMain(argc, argv); /* does not return */
> > >   else if (argc > 1 && strcmp(argv[1], "--describe-config") == 0)
> > >
> > > How about expanding that comment just a little bit, say:
> > >
> > >     /*
> > >      * Bootstrapping is handled by AuxiliaryProcessMain() for historic
> > >      * reasons.
> > >      */
>
> Do you any thoughts on this suggestion?

Sorry, I missed your suggestion.
The purpose of a comment here is to direct hackers to initdb.c because
the --boot option is used only by initdb.
initdb.c describes why it uses the --boot option (i.e., historical
reason), so I think it should not be described in main.c.

Regards,

-- 
Youki Shiraishi
NTT Software Innovation Center
Phone: +81-(0)3-5860-5115
Email: shiraishi@computer.org

On Fri, Sep 27, 2019 at 12:29:08PM +0900, Youki Shiraishi wrote:
> I also vote to get rid of such ambiguous stuff.
> As you can see by grepping, "bootstrap-mode backend" (and something
> like that) is also called in the sources as:
>
> - bootstrap backend
> - (basic) bootstrap process
> - backend running in bootstrap mode
> - postgres (backend) program in bootstrap mode
> - bootstrapper
>
> I think "bootstrap backend" is a strong candidate for an alternative
> of "bootstrapper" because it is used in the official documentation of
> initdb.

It seems to me that "backend running in bootstrap mode" would be the
most consistent way to define that state in a backend process:
$ git grep -i "bootstrap mode backend" | wc -l
0
$ git grep -i "bootstrap-mode" | wc -l
0
$ git grep -i "bootstrap mode" | wc -l
68
$ git grep -i "bootstrap process" | wc -l
9

"bootstrapper" sounds weird.  My 2c.
--
Michael

On Fri, Sep 27, 2019 at 1:03 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Fri, Sep 27, 2019 at 12:29:08PM +0900, Youki Shiraishi wrote:
> > I also vote to get rid of such ambiguous stuff.
> > As you can see by grepping, "bootstrap-mode backend" (and something
> > like that) is also called in the sources as:
> >
> > - bootstrap backend
> > - (basic) bootstrap process
> > - backend running in bootstrap mode
> > - postgres (backend) program in bootstrap mode
> > - bootstrapper
> >
> > I think "bootstrap backend" is a strong candidate for an alternative
> > of "bootstrapper" because it is used in the official documentation of
> > initdb.
>
> It seems to me that "backend running in bootstrap mode" would be the
> most consistent way to define that state in a backend process:
> $ git grep -i "bootstrap mode backend" | wc -l
> 0
> $ git grep -i "bootstrap-mode" | wc -l
> 0
> $ git grep -i "bootstrap mode" | wc -l
> 68
> $ git grep -i "bootstrap process" | wc -l
> 9
>
> "bootstrapper" sounds weird.  My 2c.

Alright, count me in on the "bootstrap mode backend" side too. :)

Thanks,
Amit

On Fri, Sep 27, 2019 at 12:52 PM Youki Shiraishi <shiraishi@computer.org> wrote:
> On Fri, Sep 27, 2019 at 12:10 AM Amit Langote <amitlangote09@gmail.com> wrote:
> > On Thu, Sep 26, 2019 at 6:32 PM Youki Shiraishi <shiraishi@computer.org> wrote:
> > > On Thu, Sep 26, 2019 at 5:38 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > > > + /* Bootstrap mode for initdb */
> > > >   if (argc > 1 && strcmp(argv[1], "--boot") == 0)
> > > >   AuxiliaryProcessMain(argc, argv); /* does not return */
> > > >   else if (argc > 1 && strcmp(argv[1], "--describe-config") == 0)
> > > >
> > > > How about expanding that comment just a little bit, say:
> > > >
> > > >     /*
> > > >      * Bootstrapping is handled by AuxiliaryProcessMain() for historic
> > > >      * reasons.
> > > >      */
> >
> > Do you any thoughts on this suggestion?
>
> Sorry, I missed your suggestion.
> The purpose of a comment here is to direct hackers to initdb.c because
> the --boot option is used only by initdb.
> initdb.c describes why it uses the --boot option (i.e., historical
> reason), so I think it should not be described in main.c.

Sorry, I didn't really mean to take out the "for initdb" part.  So, I
should've suggested this

    /*
     * Bootstrap mode for initdb.  Bootstrapping is handled by
     * AuxiliaryProcessMain() for historical reasons.
     */

IMO, it would be good for this comment to say why
AuxiliaryProcessMain() is invoked here instead of, say,
PostgresMain().  "for historical reasons" may not be enough but maybe
it is.

Thanks,
Amit

Hello folks,

On Fri, Sep 27, 2019 at 1:59 PM Amit Langote <amitlangote09@gmail.com> wrote:
>
> On Fri, Sep 27, 2019 at 12:52 PM Youki Shiraishi <shiraishi@computer.org> wrote:
> > On Fri, Sep 27, 2019 at 12:10 AM Amit Langote <amitlangote09@gmail.com> wrote:
> > > On Thu, Sep 26, 2019 at 6:32 PM Youki Shiraishi <shiraishi@computer.org> wrote:
> > > > On Thu, Sep 26, 2019 at 5:38 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > > > > + /* Bootstrap mode for initdb */
> > > > >   if (argc > 1 && strcmp(argv[1], "--boot") == 0)
> > > > >   AuxiliaryProcessMain(argc, argv); /* does not return */
> > > > >   else if (argc > 1 && strcmp(argv[1], "--describe-config") == 0)
> > > > >
> > > > > How about expanding that comment just a little bit, say:
> > > > >
> > > > >     /*
> > > > >      * Bootstrapping is handled by AuxiliaryProcessMain() for historic
> > > > >      * reasons.
> > > > >      */
> > >
> > > Do you any thoughts on this suggestion?
> >
> > Sorry, I missed your suggestion.
> > The purpose of a comment here is to direct hackers to initdb.c because
> > the --boot option is used only by initdb.
> > initdb.c describes why it uses the --boot option (i.e., historical
> > reason), so I think it should not be described in main.c.
>
> Sorry, I didn't really mean to take out the "for initdb" part.  So, I
> should've suggested this
>
>     /*
>      * Bootstrap mode for initdb.  Bootstrapping is handled by
>      * AuxiliaryProcessMain() for historical reasons.
>      */
>
> IMO, it would be good for this comment to say why
> AuxiliaryProcessMain() is invoked here instead of, say,
> PostgresMain().  "for historical reasons" may not be enough but maybe
> it is.

I totally agree with that.
It might also help to tell that AuxiliaryProcessMain() is special
entry point compared to others.

According to the discussion so far, I revised my patch as attached.
I believe the patch might help beginners.

Many thanks,

-- 
Youki Shiraishi
NTT Software Innovation Center
Phone: +81-(0)3-5860-5115
Email: shiraishi@computer.org