Re: Should we improve "PID XXXX is not a PostgreSQL server process" warning for pg_terminate_backend(<>)?
On 1/11/22, 10:06 AM, "John Naylor" 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
Re: Should we improve "PID XXXX is not a PostgreSQL server process" warning for pg_terminate_backend(<>)?
On Tue, Dec 7, 2021 at 10:51 PM Bossart, Nathan wrote: > > On 12/7/21, 5:21 PM, "Bharath Rupireddy" > wrote: > > On Wed, Dec 8, 2021 at 4:17 AM Bossart, Nathan 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
Re: Should we improve "PID XXXX is not a PostgreSQL server process" warning for pg_terminate_backend(<>)?
On 12/7/21, 5:21 PM, "Bharath Rupireddy" wrote: > On Wed, Dec 8, 2021 at 4:17 AM Bossart, Nathan 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
Re: Should we improve "PID XXXX is not a PostgreSQL server process" warning for pg_terminate_backend(<>)?
On Wed, Dec 8, 2021 at 4:17 AM Bossart, Nathan wrote: > > On 11/18/21, 8:27 PM, "Bharath Rupireddy" > 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.
Re: Should we improve "PID XXXX is not a PostgreSQL server process" warning for pg_terminate_backend(<>)?
On Thu, Nov 18, 2021 at 5:01 PM Bharath Rupireddy
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.
From 24800ffa999f527ac9c526ef11e6679d18269d39 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy
Date: Fri, 19 Nov 2021 02:09:49 +
Subject: [PATCH v4] Improve PID is not a PostgreSQL server process
message
Currently, pg_signal_backend emits generic WARNING "PID is
not a PostgreSQL server process" for postmaster and auxiliary
processes which doesn't sound sensible. This patch tries to
improve this message.
---
src/backend/storage/ipc/signalfuncs.c | 38 +++
1 file changed, 33 insertions(+), 5 deletions(-)
diff --git a/src/backend/storage/ipc/signalfuncs.c b/src/backend/storage/ipc/signalfuncs.c
index de69d60e79..8d0f0fea65 100644
--- a/src/backend/storage/ipc/signalfuncs.c
+++ b/src/backend/storage/ipc/signalfuncs.c
@@ -48,7 +48,17 @@
static int
pg_signal_backend(int pid, int sig)
{
- PGPROC *proc = BackendPidGetProc(pid);
+ PGPROC *proc;
+
+ if (PostmasterPid == pid)
+ {
+ ereport(WARNING,
+(errmsg("cannot send signal to postmaster %d", pid)));
+
+ return SIGNAL_BACKEND_ERROR;
+ }
+
+ proc = BackendPidGetProc(pid);
/*
* BackendPidGetProc returns NULL if the pid isn't valid; but by the time
@@ -61,11 +71,29 @@ pg_signal_backend(int pid, int sig)
if (proc == NULL)
{
/*
- * This is just a warning so a loop-through-resultset will not abort
- * if one backend terminated on its own during the run.
+ * AuxiliaryProcs are still PostgreSQL server processes, do a little
+ * more work and report a proper warning that says we cannot signal
+ * them.
+ *
+ * For an auxiliary process, retrieve process info from AuxiliaryProcs
+ * stored in shared-memory.
*/
- ereport(WARNING,
-(errmsg("PID %d is not a PostgreSQL server process", pid)));
+ proc = AuxiliaryPidGetProc(pid);
+
+ if (proc)
+ ereport(WARNING,
+ (errmsg("cannot send signal to PostgreSQL server process %d",
+ pid)));
+ else
+ {
+ /*
+ * This is just a warning so a loop-through-resultset will not
+ * abort if one backend terminated on its own during the run.
+ */
+ ereport(WARNING,
+ (errmsg("PID %d is not a PostgreSQL backend process", pid)));
+ }
+
return SIGNAL_BACKEND_ERROR;
}
--
2.25.1
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 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.
Re: Should we improve "PID XXXX is not a PostgreSQL server process" warning for pg_terminate_backend(<>)?
Justin Pryzby 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
Re: Should we improve "PID XXXX is not a PostgreSQL server process" warning for pg_terminate_backend(<>)?
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
Re: Should we improve "PID XXXX is not a PostgreSQL server process" warning for pg_terminate_backend(<>)?
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/
Re: Should we improve "PID XXXX is not a PostgreSQL server process" warning for pg_terminate_backend(<>)?
On Sun, Mar 7, 2021 at 3:46 PM Bharath Rupireddy wrote: > > On Fri, Feb 5, 2021 at 5:15 PM Bharath Rupireddy > wrote: > > > > pg_terminate_backend and pg_cancel_backend with postmaster PID produce > > "PID 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 X 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. v3-0001-Improve-PID--is-not-a-PostgreSQL-server-proce.patch Description: Binary data
Re: Should we improve "PID XXXX is not a PostgreSQL server process" warning for pg_terminate_backend(<>)?
On Wed, Mar 17, 2021 at 8:05 AM torikoshia 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 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 is not a PostgreSQL server process" to "PID 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 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 v2-0001-Improve-PID--is-not-a-PostgreSQL-server-proce.patch Description: Binary data
Re: Should we improve "PID XXXX is not a PostgreSQL server process" warning for pg_terminate_backend(<>)?
On 2021-03-16 20:51, Bharath Rupireddy wrote: On Mon, Mar 15, 2021 at 11:23 AM torikoshia wrote: On 2021-03-07 19:16, Bharath Rupireddy wrote: > On Fri, Feb 5, 2021 at 5:15 PM Bharath Rupireddy > wrote: >> >> pg_terminate_backend and pg_cancel_backend with postmaster PID produce >> "PID 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 X 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 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 is not a PostgreSQL server process" to "PID 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 is not a PostgreSQL server process" does not seem to cause such problems. Regards, -- Atsushi Torikoshi NTT DATA CORPORATION
Re: Should we improve "PID XXXX is not a PostgreSQL server process" warning for pg_terminate_backend(<>)?
On Mon, Mar 15, 2021 at 11:23 AM torikoshia wrote: > > On 2021-03-07 19:16, Bharath Rupireddy wrote: > > On Fri, Feb 5, 2021 at 5:15 PM Bharath Rupireddy > > wrote: > >> > >> pg_terminate_backend and pg_cancel_backend with postmaster PID produce > >> "PID 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 X 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 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 is not a PostgreSQL server process" to "PID 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
Re: Should we improve "PID XXXX is not a PostgreSQL server process" warning for pg_terminate_backend(<>)?
On 2021-03-07 19:16, Bharath Rupireddy wrote: On Fri, Feb 5, 2021 at 5:15 PM Bharath Rupireddy wrote: pg_terminate_backend and pg_cancel_backend with postmaster PID produce "PID 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]. 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 X 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'm now wondering if changing the message to something like "PID 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] https://www.postgresql.org/message-id/CALDaNm3ZzmFS-%3Dr7oDUzj7y7BgQv%2BN06Kqyft6C3xZDoKnk_6w%40mail.gmail.com [2] https://www.postgresql.org/message-id/0271f440ac77f2a4180e0e56ebd944d1%40oss.nttdata.com Regards, -- Atsushi Torikoshi NTT DATA CORPORATION
Re: Should we improve "PID XXXX is not a PostgreSQL server process" warning for pg_terminate_backend(<>)?
On Fri, Feb 5, 2021 at 5:15 PM Bharath Rupireddy wrote: > > pg_terminate_backend and pg_cancel_backend with postmaster PID produce > "PID 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 X 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 v1-0001-Improve-PID--is-not-a-PostgreSQL-server-proce.patch Description: Binary data
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 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
