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

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] 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")));

Regards,
Bharath Rupireddy.