Обсуждение: Should we improve "PID XXXX is not a PostgreSQL server process" warning for pg_terminate_backend(<>)?

Hi,

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)

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], I'm attaching a
new v3 patch here. Please review it.

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

[1] - https://www.postgresql.org/message-id/CAFiTN-sX_66svOPdix1edB_WxGj%3DWu4XouyRQrvySwCK0V8Btg%40mail.gmail.com

Regards,
Bharath Rupireddy.

As there is some interest shown in this thread at [1], I'm attaching a

new v3 patch here. Please review it.

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] 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.

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].
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(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