Subject: Re: drop postmaster symlink

From: "Karl O. Pinc" <[email-masked]>
Date: Sat, 7 Jan 2023 16:59:42 -0600

Hello,

This is a review of Peter's 2 patches.  I see only 1 small problem.

                           +++

Looking at the documentation, a "postmaster" in the glossary is
defined as the controlling process.  This works; it needs to be called
something.  There is still a postmaster.pid (etc.) in the data
directory.

The word "postmaster" (case insensitive) shows up 84 times in the
documentation.  I looked at all of these.  

I see a possible problem at line 1,412 of runtime.sgml, the "Linux
Memory Overcommit" section.  It talks about the postmaster's startup
script invoking the postmaster.  It might, possibly, be better to say
that "postgres" is invoked, that being the name of the invoked program.
There's a similar usage at line 1,416. On the other hand, the existing
text makes it quite clear what is going on and there's a lot to be said
for consistently using the word "postmaster".  I mention this only in
case somebody deems it significant.  Perhaps there's a way to use
markup, identifying "postgres" as a program with a name in the file
system, to make things more clear.  Most likely, nobody will care.

In doc/src/sgml/ref/allfiles.sgml at line 222 there is an ENTITY
defined which references the deleted postmaster.sgml file.  Since I
did a maintainer-clean, and the patch deletes the postmaster.sgml
file, and I don't see any references to the entity in the docs, I
believe that this line should be removed.  (In other words, I don't
think this file is automatically maintained.)

After applying the patches the documentation seems to build just fine.

I have not run contrib/start-scripts/{freebsd,linux}, but the patch
looks fine and the result of applying the patch looks fine, and the
patch is a one-line simple replacement of "postmaster" with "postgres"
so I expect no problems.

The modification to src/port/path.c is in a comment, so it will surely
not affect anything at runtime.  And the changed comment looks right
to me.

There's thousands of lines of comments in the code where "postmaster"
(case insensitive) shows up after applying the patches.  I've not
reviewed any of these.  Or looked for odd variable names, or
references in the code to the postmaster binary - by name, etc.  I'm
not sure where it would make sense to look for such problems.

Aside from the "allfiles.sgml" problem, I see no reason why these 2
patches should not be applied.  As mentioned by others, I don't have
strong feelings about getting rid of the postmaster symlink.  But I
did pick this patch to review because I remember, once upon a time,
being slightly confused by a "postmaster" in the process list.

Regards,

Karl <[email-masked]>
Free Software:  "You don't pay back, you pay forward."
                 -- Robert A. Heinlein

---

From: Tom Lane <[email-masked]>
Date: Sat, 07 Jan 2023 18:38:25 -0500

> documentation.  I looked at all of these.  

Hmm ... I thought this patch was about getting rid of the
admittedly-obsolete installed symlink.  If it's trying to get rid of
the "postmaster" terminology for our parent process, I'm very strongly
against that, either as regards to code or documentation.

			regards, tom lane

---

From: Joe Conway <[email-masked]>
Date: Sat, 7 Jan 2023 19:33:38 -0500

On 1/7/23 18:38, Tom Lane wrote:
> admittedly-obsolete installed symlink.

That was my understanding too.

> If it's trying to get rid of the "postmaster" terminology for our
> parent process, I'm very strongly against that, either as regards to
> code or documentation.

+1

-- 
Joe Conway
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

---

From: "Karl O. Pinc" <[email-masked]>
Date: Sat, 7 Jan 2023 19:56:08 -0600

On Sat, 07 Jan 2023 18:38:25 -0500
Tom Lane <[email-masked]> wrote:

> > defined as the controlling process.  This works; it needs to be
> > called something.  There is still a postmaster.pid (etc.) in the
> > data directory.  
> 

No.  It's about getting rid of the symlink.  I was grepping around
to look for use of the symlink, started with the glossary just
to be sure, and saw that "postmaster" is consistently (I think)
used to refer to the controlling, parent, process.  And wrote
down what I was doing and what I found as I went along.  And then
sent out my notes.

Sorry for the confusion.

Regards,

Karl <[email-masked]>

---

From: "Karl O. Pinc" <[email-masked]>
Date: Sat, 7 Jan 2023 19:57:09 -0600

On Sat, 7 Jan 2023 19:33:38 -0500

> >> This is a review of Peter's 2 patches.  I see only 1 small

The small problem is a reference to a deleted file.

Regards,

Karl <[email-masked]>

---

From: "Karl O. Pinc" <[email-masked]>
Date: Sat, 7 Jan 2023 22:29:35 -0600

On Sat, 7 Jan 2023 19:56:08 -0600
"Karl O. Pinc" <[email-masked]> wrote:

> > > This is a review of Peter's 2 patches.  I see only 1 small
> > > problem. ...

> > admittedly-obsolete installed symlink. ...

> No.  It's about getting rid of the symlink. 

The only way I could think of to review a patch
that removes something is to report all the places
I looked where a reference to the symlink might be.
Then report what I found each place I looked and
report if there's a problem, or might be.

That way the committer knows where I didn't look if there's
more that needs removing.

Apologies that this was not clear.

Regards,

Karl <[email-masked]>

---

From: "Karl O. Pinc" <[email-masked]>
Date: Sun, 8 Jan 2023 14:17:00 -0600

On Sat, 7 Jan 2023 22:29:35 -0600

> I looked where a reference to the symlink might be.

I forgot to report that I also tried a `make install`
and 'make uninstall`, with no problems.

FWIW, I suspect the include/backend/postmaster/ directory
would today be named include/backend/postgres/.   Now
would be the time to change the name, but I don't see
it being worth the work.  And while such a change
wouldn't break pg, that kind of change would break something
for somebody.

Regards,

Karl <[email-masked]>