On Thu, Nov 18, 2021 at 12:30 AM Euler Taveira <eu...@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.