Should we improve "PID XXXX is not a PostgreSQL server process" warning for pg_terminate_backend(<<postmaster_pid>>)?

On Fri, Feb 5, 2021 at 5:15 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

pg_terminate_backend and pg_cancel_backend with postmaster PID produce
"PID XXXX is not a PostgresSQL server process" warning [1], which
basically implies that the postmaster is not a PostgreSQL process at
all. This is a bit misleading because the postmaster is the parent of
all PostgreSQL processes. Should we improve the warning message if the
given PID is postmasters' PID?

If yes, how about a generic message for both of the functions -
"signalling postmaster process is not allowed" or "cannot signal
postmaster process" or some other better suggestion?

[1] 2471176 ---> is postmaster PID.
postgres=# select pg_terminate_backend(2471176);
WARNING: PID 2471176 is not a PostgreSQL server process
pg_terminate_backend
----------------------
f
(1 row)
postgres=# select pg_cancel_backend(2471176);
WARNING: PID 2471176 is not a PostgreSQL server process
pg_cancel_backend
-------------------
f
(1 row)

I'm attaching a small patch that emits a warning "signalling
postmaster with PID %d is not allowed" for postmaster and "signalling
PostgreSQL server process with PID %d is not allowed" for auxiliary
processes such as checkpointer, background writer, walwriter.

However, for stats collector and sys logger processes, we still get
"PID XXXXX is not a PostgreSQL server process" warning because they
don't have PGPROC entries(??). So BackendPidGetProc and
AuxiliaryPidGetProc will not help and even pg_stat_activity is not
having these processes' pid.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On 2021-03-07 19:16, Bharath Rupireddy wrote:

On Fri, Feb 5, 2021 at 5:15 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

pg_terminate_backend and pg_cancel_backend with postmaster PID produce
"PID XXXX is not a PostgresSQL server process" warning [1], which
basically implies that the postmaster is not a PostgreSQL process at
all. This is a bit misleading because the postmaster is the parent of
all PostgreSQL processes. Should we improve the warning message if the
given PID is postmasters' PID?

+1. I felt it was a bit confusing when reviewing a thread[1]/messages/by-id/CALDaNm3ZzmFS-=r7oDUzj7y7BgQv+N06Kqyft6C3xZDoKnk_6w@mail.gmail.com.

If yes, how about a generic message for both of the functions -
"signalling postmaster process is not allowed" or "cannot signal
postmaster process" or some other better suggestion?

[1] 2471176 ---> is postmaster PID.
postgres=# select pg_terminate_backend(2471176);
WARNING: PID 2471176 is not a PostgreSQL server process
pg_terminate_backend
----------------------
f
(1 row)
postgres=# select pg_cancel_backend(2471176);
WARNING: PID 2471176 is not a PostgreSQL server process
pg_cancel_backend
-------------------
f
(1 row)

I'm attaching a small patch that emits a warning "signalling
postmaster with PID %d is not allowed" for postmaster and "signalling
PostgreSQL server process with PID %d is not allowed" for auxiliary
processes such as checkpointer, background writer, walwriter.

However, for stats collector and sys logger processes, we still get
"PID XXXXX is not a PostgreSQL server process" warning because they
don't have PGPROC entries(??). So BackendPidGetProc and
AuxiliaryPidGetProc will not help and even pg_stat_activity is not
having these processes' pid.

I also ran into the same problem while creating a patch in [2]/messages/by-id/0271f440ac77f2a4180e0e56ebd944d1@oss.nttdata.com.

I'm now wondering if changing the message to something like
"PID XXXX is not a PostgreSQL backend process".

"backend process' is now defined as "Process of an instance
which acts on behalf of a client session and handles its
requests." in Appendix.

[1]: /messages/by-id/CALDaNm3ZzmFS-=r7oDUzj7y7BgQv+N06Kqyft6C3xZDoKnk_6w@mail.gmail.com
/messages/by-id/CALDaNm3ZzmFS-=r7oDUzj7y7BgQv+N06Kqyft6C3xZDoKnk_6w@mail.gmail.com

[2]: /messages/by-id/0271f440ac77f2a4180e0e56ebd944d1@oss.nttdata.com
/messages/by-id/0271f440ac77f2a4180e0e56ebd944d1@oss.nttdata.com

Regards,

--
Atsushi Torikoshi
NTT DATA CORPORATION

On Mon, Mar 15, 2021 at 11:23 AM torikoshia <torikoshia@oss.nttdata.com> wrote:

On 2021-03-07 19:16, Bharath Rupireddy wrote:

On Fri, Feb 5, 2021 at 5:15 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

pg_terminate_backend and pg_cancel_backend with postmaster PID produce
"PID XXXX is not a PostgresSQL server process" warning [1], which
basically implies that the postmaster is not a PostgreSQL process at
all. This is a bit misleading because the postmaster is the parent of
all PostgreSQL processes. Should we improve the warning message if the
given PID is postmasters' PID?

+1. I felt it was a bit confusing when reviewing a thread[1].

Hmmm.

I'm attaching a small patch that emits a warning "signalling
postmaster with PID %d is not allowed" for postmaster and "signalling
PostgreSQL server process with PID %d is not allowed" for auxiliary
processes such as checkpointer, background writer, walwriter.

However, for stats collector and sys logger processes, we still get
"PID XXXXX is not a PostgreSQL server process" warning because they
don't have PGPROC entries(??). So BackendPidGetProc and
AuxiliaryPidGetProc will not help and even pg_stat_activity is not
having these processes' pid.

I also ran into the same problem while creating a patch in [2].

I have not gone through that thread though. Is there any way we can
detect those child processes(stats collector, sys logger) that are
forked by the postmaster from a backend process? Thoughts?

I'm now wondering if changing the message to something like
"PID XXXX is not a PostgreSQL backend process".

"backend process' is now defined as "Process of an instance
which acts on behalf of a client session and handles its
requests." in Appendix.

Yeah, that looks good to me. IIUC, we can just change the message from
"PID XXXX is not a PostgreSQL server process" to "PID XXXX is not a
PostgreSQL backend process" and we don't need look for AuxiliaryProcs
or PostmasterPid.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On 2021-03-16 20:51, Bharath Rupireddy wrote:

On Mon, Mar 15, 2021 at 11:23 AM torikoshia
<torikoshia@oss.nttdata.com> wrote:

On 2021-03-07 19:16, Bharath Rupireddy wrote:

On Fri, Feb 5, 2021 at 5:15 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

pg_terminate_backend and pg_cancel_backend with postmaster PID produce
"PID XXXX is not a PostgresSQL server process" warning [1], which
basically implies that the postmaster is not a PostgreSQL process at
all. This is a bit misleading because the postmaster is the parent of
all PostgreSQL processes. Should we improve the warning message if the
given PID is postmasters' PID?

+1. I felt it was a bit confusing when reviewing a thread[1].

Hmmm.

I'm attaching a small patch that emits a warning "signalling
postmaster with PID %d is not allowed" for postmaster and "signalling
PostgreSQL server process with PID %d is not allowed" for auxiliary
processes such as checkpointer, background writer, walwriter.

However, for stats collector and sys logger processes, we still get
"PID XXXXX is not a PostgreSQL server process" warning because they
don't have PGPROC entries(??). So BackendPidGetProc and
AuxiliaryPidGetProc will not help and even pg_stat_activity is not
having these processes' pid.

I also ran into the same problem while creating a patch in [2].

I have not gone through that thread though. Is there any way we can
detect those child processes(stats collector, sys logger) that are
forked by the postmaster from a backend process? Thoughts?

I couldn't find good ways to do that, and thus I'm now wondering
just changing the message.

I'm now wondering if changing the message to something like
"PID XXXX is not a PostgreSQL backend process".

"backend process' is now defined as "Process of an instance
which acts on behalf of a client session and handles its
requests." in Appendix.

Yeah, that looks good to me. IIUC, we can just change the message from
"PID XXXX is not a PostgreSQL server process" to "PID XXXX is not a
PostgreSQL backend process" and we don't need look for AuxiliaryProcs
or PostmasterPid.

Changing log messages can affect operations, especially when people
monitor the log message strings, but improving "PID XXXX is not a
PostgreSQL server process" does not seem to cause such problems.

Regards,

--
Atsushi Torikoshi
NTT DATA CORPORATION

On Wed, Mar 17, 2021 at 8:05 AM torikoshia <torikoshia@oss.nttdata.com> wrote:

I have not gone through that thread though. Is there any way we can
detect those child processes(stats collector, sys logger) that are
forked by the postmaster from a backend process? Thoughts?

I couldn't find good ways to do that, and thus I'm now wondering
just changing the message.

Changed the message.

I'm now wondering if changing the message to something like
"PID XXXX is not a PostgreSQL backend process".

"backend process' is now defined as "Process of an instance
which acts on behalf of a client session and handles its
requests." in Appendix.

Yeah, that looks good to me. IIUC, we can just change the message from
"PID XXXX is not a PostgreSQL server process" to "PID XXXX is not a
PostgreSQL backend process" and we don't need look for AuxiliaryProcs
or PostmasterPid.

Changing log messages can affect operations, especially when people
monitor the log message strings, but improving "PID XXXX is not a
PostgreSQL server process" does not seem to cause such problems.

Now the error message clearly says that the given pid not a backend
process. Attaching v2 patch. Please have a look.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Sun, Mar 7, 2021 at 3:46 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

On Fri, Feb 5, 2021 at 5:15 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

pg_terminate_backend and pg_cancel_backend with postmaster PID produce
"PID XXXX is not a PostgresSQL server process" warning [1], which
basically implies that the postmaster is not a PostgreSQL process at
all. This is a bit misleading because the postmaster is the parent of
all PostgreSQL processes. Should we improve the warning message if the
given PID is postmasters' PID?

If yes, how about a generic message for both of the functions -
"signalling postmaster process is not allowed" or "cannot signal
postmaster process" or some other better suggestion?

[1] 2471176 ---> is postmaster PID.
postgres=# select pg_terminate_backend(2471176);
WARNING: PID 2471176 is not a PostgreSQL server process
pg_terminate_backend
----------------------
f
(1 row)
postgres=# select pg_cancel_backend(2471176);
WARNING: PID 2471176 is not a PostgreSQL server process
pg_cancel_backend
-------------------
f
(1 row)

I'm attaching a small patch that emits a warning "signalling
postmaster with PID %d is not allowed" for postmaster and "signalling
PostgreSQL server process with PID %d is not allowed" for auxiliary
processes such as checkpointer, background writer, walwriter.

However, for stats collector and sys logger processes, we still get
"PID XXXXX is not a PostgreSQL server process" warning because they
don't have PGPROC entries(??). So BackendPidGetProc and
AuxiliaryPidGetProc will not help and even pg_stat_activity is not
having these processes' pid.

As there is some interest shown in this thread at [1]/messages/by-id/CAFiTN-sX_66svOPdix1edB_WxGj=Wu4XouyRQrvySwCK0V8Btg@mail.gmail.com, I'm attaching a
new v3 patch here. Please review it.

CF entry - https://commitfest.postgresql.org/36/3411/

[1]: /messages/by-id/CAFiTN-sX_66svOPdix1edB_WxGj=Wu4XouyRQrvySwCK0V8Btg@mail.gmail.com

Regards,
Bharath Rupireddy.

On Mon, Nov 15, 2021, at 4:27 AM, Bharath Rupireddy wrote:

As there is some interest shown in this thread at [1], I'm attaching a
new v3 patch here. Please review it.

I took a look at this patch. I have a few comments.

+ ereport(WARNING,
+ (errmsg("signalling postmaster with PID %d is not allowed", pid)));

I would say "signal postmaster PID 1234 is not allowed". It is not an
in-progress action.

s/shared-memory/shared memory/

syslogger and statistics collector don't have a procArray entry so you could
probably provide a new function that checks if it is an auxiliary process.
AuxiliaryPidGetProc() does not return all auxiliary processes; syslogger and
statistics collector don't have a procArray entry. You can use their PIDs
(SysLoggerPID and PgStatPID) to provide an accurate information.

+ if (proc)
+ ereport(WARNING,
+ (errmsg("signalling PostgreSQL server process with PID %d is not allowed",

I would say "signal PostgreSQL auxiliary process PID 1234 is not allowed".

+ ereport(WARNING,
+ (errmsg("PID %d is not a PostgreSQL server process", pid)));

I would say "PID 1234 is not a PostgreSQL backend process". That's the glossary
terminology.

--
Euler Taveira
EDB https://www.enterprisedb.com/

On Wed, Nov 17, 2021 at 03:59:59PM -0300, Euler Taveira wrote:

On Mon, Nov 15, 2021, at 4:27 AM, Bharath Rupireddy wrote:

As there is some interest shown in this thread at [1], I'm attaching a
new v3 patch here. Please review it.

I took a look at this patch. I have a few comments.

+ ereport(WARNING,
+ (errmsg("signalling postmaster with PID %d is not allowed", pid)));

I would say "signal postmaster PID 1234 is not allowed". It is not an
in-progress action.

It's correct to say "signalling ... is not allowed", which means the same as
"it is not allowed to signal ...".

--
Justin

Justin Pryzby <pryzby@telsasoft.com> writes:

On Wed, Nov 17, 2021 at 03:59:59PM -0300, Euler Taveira wrote:

I took a look at this patch. I have a few comments.

+ ereport(WARNING,
+ (errmsg("signalling postmaster with PID %d is not allowed", pid)));

I would say "signal postmaster PID 1234 is not allowed". It is not an
in-progress action.

It's correct to say "signalling ... is not allowed", which means the same as
"it is not allowed to signal ...".

Yeah, the grammar is fine as far as that goes. What reads awkwardly to me
is inclusion of "with PID %d" in the middle of the sentence. That seems
odd, not least because it leaves the impression that maybe it would've
been okay to signal some other postmaster with a different PID.

Frankly, I think the existing wording is fine and this patch adds
complication without making any useful improvement. We could maybe change
"is not a PostgresSQL server process" to "is not a PostgresSQL backend
process", but I wouldn't go further than that.

regards, tom lane

On Thu, Nov 18, 2021 at 12:30 AM Euler Taveira <euler@eulerto.com> wrote:

On Mon, Nov 15, 2021, at 4:27 AM, Bharath Rupireddy wrote:

As there is some interest shown in this thread at [1], I'm attaching a
new v3 patch here. Please review it.

I took a look at this patch. I have a few comments.

Thanks a lot.

s/shared-memory/shared memory/

I don't think we need to change that. This comment is picked up from
another AuxiliaryPidGetProc call from pgstatsfuncs.c and in the core
we have lots of instances of the term "shared-memory". I think we can
have it as is and let's not attempt to change it here in this thread
at least.

syslogger and statistics collector don't have a procArray entry so you could
probably provide a new function that checks if it is an auxiliary process.
AuxiliaryPidGetProc() does not return all auxiliary processes; syslogger and
statistics collector don't have a procArray entry. You can use their PIDs
(SysLoggerPID and PgStatPID) to provide an accurate information.

+ if (proc)
+ ereport(WARNING,
+ (errmsg("signalling PostgreSQL server process with PID %d is not allowed",

I would say "signal PostgreSQL auxiliary process PID 1234 is not allowed".

Although we have defined the term auxiliary process in the glossary
recently, I haven't found (on a quick look) any user facing log
messages using the term "auxiliary process". And if we just say "we
can't signal an auxiliary process", it doesn't look complete (we end
up hitting the other messages down for syslogger and stats collector).
Note that the AuxiliaryPidGetProc doesn't return a PGPROC entry for
syslogger and stats collector which according to the glossary are
auxiliary processes.

+ ereport(WARNING,
+ (errmsg("PID %d is not a PostgreSQL server process", pid)));

I would say "PID 1234 is not a PostgreSQL backend process". That's the glossary
terminology.

This looks okay as it along with the other new messages, says that the
calling function is allowed only to signal the backend process not the
postmaster or the other postgresql process (auxiliary process) or the
non-postgres processes.

The following is what I made up in my mind after looking at other
existing messages, like [1](errmsg("could not send signal to process %d: %m", pid))); (errmsg("failed to send signal to postmaster: %m"))); and the review comments:
errmsg("cannot send signal to postmaster %d", pid, --> the process
is postmaster but the caller isn't allowed to signal.
errmsg("cannot send signal to PostgreSQL server process %d", pid,
--> the process is a postgresql process but the caller isn't allowed
to signal.
errmsg("PID %d is not a PostgreSQL backend process", pid, ---> it may
be another postgres processes like syslogger or stats collector or
non-postgres process but not a backend process.

Thoughts?

[1]: (errmsg("could not send signal to process %d: %m", pid))); (errmsg("failed to send signal to postmaster: %m")));
(errmsg("could not send signal to process %d: %m", pid)));
(errmsg("failed to send signal to postmaster: %m")));

Regards,
Bharath Rupireddy.

On Thu, Nov 18, 2021 at 5:01 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

The following is what I made up in my mind after looking at other
existing messages, like [1] and the review comments:
errmsg("cannot send signal to postmaster %d", pid, --> the process
is postmaster but the caller isn't allowed to signal.
errmsg("cannot send signal to PostgreSQL server process %d", pid,
--> the process is a postgresql process but the caller isn't allowed
to signal.
errmsg("PID %d is not a PostgreSQL backend process", pid, ---> it may
be another postgres processes like syslogger or stats collector or
non-postgres process but not a backend process.

Thoughts?

[1]
(errmsg("could not send signal to process %d: %m", pid)));
(errmsg("failed to send signal to postmaster: %m")));

Here's the v4 patch with the above changes, the output looks like [1]postgres=# select pg_terminate_backend(2407245); WARNING: cannot send signal to postmaster 2407245 pg_terminate_backend ---------------------- f (1 row).
Please review it further.

[1]: postgres=# select pg_terminate_backend(2407245); WARNING: cannot send signal to postmaster 2407245 pg_terminate_backend ---------------------- f (1 row)
postgres=# select pg_terminate_backend(2407245);
WARNING: cannot send signal to postmaster 2407245
pg_terminate_backend
----------------------
f
(1 row)

postgres=# select pg_terminate_backend(2407246);
WARNING: cannot send signal to PostgreSQL server process 2407246
pg_terminate_backend
----------------------
f
(1 row)

postgres=# select pg_terminate_backend(2407286);
WARNING: PID 2407286 is not a PostgreSQL backend process
pg_terminate_backend
----------------------
f
(1 row)

Regards,
Bharath Rupireddy.

On 11/18/21, 8:27 PM, "Bharath Rupireddy" <bharath.rupireddyforpostgres@gmail.com> wrote:

Here's the v4 patch with the above changes, the output looks like [1].
Please review it further.

I agree with Tom. I would just s/server/backend/ (as per the
attached) and call it a day.

Nathan

On Wed, Dec 8, 2021 at 4:17 AM Bossart, Nathan <bossartn@amazon.com> wrote:

On 11/18/21, 8:27 PM, "Bharath Rupireddy" <bharath.rupireddyforpostgres@gmail.com> wrote:

Here's the v4 patch with the above changes, the output looks like [1].
Please review it further.

I agree with Tom. I would just s/server/backend/ (as per the
attached) and call it a day.

Thanks. v5 patch looks good to me.

Regards,
Bharath Rupireddy.

On 12/7/21, 5:21 PM, "Bharath Rupireddy" <bharath.rupireddyforpostgres@gmail.com> wrote:

On Wed, Dec 8, 2021 at 4:17 AM Bossart, Nathan <bossartn@amazon.com> wrote:

I agree with Tom. I would just s/server/backend/ (as per the
attached) and call it a day.

Thanks. v5 patch looks good to me.

I've marked the commitfest entry as ready-for-committer.

Nathan

On Tue, Dec 7, 2021 at 10:51 PM Bossart, Nathan <bossartn@amazon.com> wrote:

On 12/7/21, 5:21 PM, "Bharath Rupireddy" <bharath.rupireddyforpostgres@gmail.com> wrote:

On Wed, Dec 8, 2021 at 4:17 AM Bossart, Nathan <bossartn@amazon.com> wrote:

I agree with Tom. I would just s/server/backend/ (as per the
attached) and call it a day.

Thanks. v5 patch looks good to me.

I've marked the commitfest entry as ready-for-committer.

I pushed this with one small change -- I felt the comment didn't need
to explain the warning message, since it now simply matches the coding
more exactly. Also, v5 was a big enough change from v4 that I put
Nathan as the first author.

--
John Naylor
EDB: http://www.enterprisedb.com

On 1/11/22, 10:06 AM, "John Naylor" <john.naylor@enterprisedb.com> wrote:

I pushed this with one small change -- I felt the comment didn't need
to explain the warning message, since it now simply matches the coding
more exactly. Also, v5 was a big enough change from v4 that I put
Nathan as the first author.

Thanks!

Nathan