On Fri, Dec 16, 2011 at 13:31, Greg Smith <g...@2ndquadrant.com> wrote:
> On 12/14/2011 05:24 AM, Magnus Hagander wrote:
>> How about passing a parameter to pg_signal_backend? Making
>> pg_signal_backend(int pid, int sig, bool allow_samerole)?
> That works, got rid of the parts I didn't like and allowed some useful minor
> restructuring.  I also made the HINT better and match style guidelines:
> gsmith=> select pg_terminate_backend(21205);
> ERROR:  must be superuser to terminate other server processes
> HINT:  You can cancel your own processes with pg_cancel_backend().
> gsmith=> select pg_cancel_backend(21205);
>  pg_cancel_backend
> -------------------
>  t
> New rev attached and pushed to
> https://github.com/greg2ndQuadrant/postgres/tree/cancel-backend (which is
> *not* the same branch as I used last time; don't ask why, long story)
> I considered some additional ways to restructure the checks that could
> remove a further line or two from the logic here, but they all made the
> result seem less readable to me.  And this is not a high performance code
> path.  I may have gone a bit too far with the comment additions though, so
> feel free to trim that back.  It kept feeling weird to me that none of the
> individual signaling functions had their own intro comments.  I added all
> those.
> I also wrote up a commentary on the PID wraparound race condition
> possibility Josh brought up.  Some research shows that pid assignment on
> some systems is made more secure by assigning new ones randomly.  That seems
> like it would make it possible to have a pid get reused much faster than on
> the usual sort of system that does sequential assignment and wraparound.  A
> reuse collision still seems extremely unlikely though.  With the new
> comments, at least a future someone who speculates on this will know how
> much thinking went into the current implementation:  enough to notice, not
> enough to see anything worth doing about it.  Maybe that's just wasted lines
> of text?
> With so little grief on the last round, I'm going to guess this one will
> just get picked up by Magnus to commit next.  Marking accordingly and moved
> to the current CommitFest.

I was going to, but I noticed a few things:

* I restructured the if statements, because I had a hard time
following the comments around that ;) I find this one easier - but I'm
happy to change back if you think your version was more readable.

* The error message in pg_signal_backend breaks the abstraction,
because it specifically talks about terminating the other backend -
when it's not supposed to know about that in that function. I think we
either need to get rid of the hint completely, or we need to find a
way to issue it from the caller. Or pass it as a parameter. It's fine
for now since we only have two signals, but we might have more in the

* I gave it a run of pgindent ;)

In the attached updated patch I've just removed the HINT and changed
the reference from"terminate" to "signal". But I'd like your input
onthat before I commit :-)

 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index e7f7fe0..cf77586 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -14244,8 +14244,8 @@ SELECT set_config('log_statement_stats', 'off', false);
     The functions shown in <xref
     linkend="functions-admin-signal-table"> send control signals to
-    other server processes.  Use of these functions is restricted
-    to superusers.
+    other server processes.  Use of these functions is usually restricted
+    to superusers, with noted exceptions.
    <table id="functions-admin-signal-table">
@@ -14262,7 +14262,10 @@ SELECT set_config('log_statement_stats', 'off', false);
         <literal><function>pg_cancel_backend(<parameter>pid</parameter> <type>int</>)</function></literal>
-       <entry>Cancel a backend's current query</entry>
+       <entry>Cancel a backend's current query.  You can execute this against
+        another backend that has exactly the same role as the user calling the
+        function.  In all other cases, you must be a superuser.
+        </entry>
@@ -14304,6 +14307,10 @@ SELECT set_config('log_statement_stats', 'off', false);
     <command>postgres</command> processes on the server (using
     <application>ps</> on Unix or the <application>Task
     Manager</> on <productname>Windows</>).
+    For the less restrictive <function>pg_cancel_backend</>, the role of an
+    active backend can be found from
+    the <structfield>usename</structfield> column of the
+    <structname>pg_stat_activity</structname> view.
diff --git a/src/backend/utils/adt/misc.c b/src/backend/utils/adt/misc.c
index 7a2e0c8..d7f2435 100644
--- a/src/backend/utils/adt/misc.c
+++ b/src/backend/utils/adt/misc.c
@@ -30,6 +30,7 @@
 #include "postmaster/syslogger.h"
 #include "storage/fd.h"
 #include "storage/pmsignal.h"
+#include "storage/proc.h"
 #include "storage/procarray.h"
 #include "tcop/tcopprot.h"
 #include "utils/builtins.h"
@@ -70,15 +71,41 @@ current_query(PG_FUNCTION_ARGS)
- * Functions to send signals to other backends.
+ * Send a signal to another backend
 static bool
-pg_signal_backend(int pid, int sig)
+pg_signal_backend(int pid, int sig, bool allow_same_role)
+	PGPROC	   *proc;
 	if (!superuser())
-		ereport(ERROR,
-			(errmsg("must be superuser to signal other server processes"))));
+	{
+		if (allow_same_role)
+		{
+			/*
+			 * When same role permission is allowed, check for matching roles.
+			 * Trust that BackendPidGetProc will return NULL if the pid isn't
+			 * valid, even though the check for whether it's a backend process
+			 * is below. The IsBackendPid check can't be relied on as
+			 * definitive even if it was first. The process might end between
+			 * successive checks regardless of their order. There's no way to
+			 * acquire a lock on an arbitrary process to prevent that. But
+			 * since so far all the callers of this mechanism involve some
+			 * request for ending the process anyway, that it might end on its
+			 * own first is not a problem.
+			 */
+			proc = BackendPidGetProc(pid);
+			if (proc == NULL || proc->roleId != GetUserId())
+				ereport(ERROR,
+						 (errmsg("must be superuser or have the same role to signal other server processes"))));
+		}
+		else
+			ereport(ERROR,
+			  errmsg("must be superuser to signal other server processes")));
+	}
 	if (!IsBackendPid(pid))
@@ -91,6 +118,15 @@ pg_signal_backend(int pid, int sig)
 		return false;
+	/*
+	 * Can the process we just validated above end, followed by the pid being
+	 * recycled for a new process, before reaching here?  Then we'd be trying
+	 * to kill the wrong thing.  Seems near impossible when sequential pid
+	 * assignment and wraparound is used.  Perhaps it could happen on a system
+	 * where pid re-use is randomized.	That race condition possibility seems
+	 * too unlikely to worry about.
+	 */
 	/* If we have setsid(), signal the backend's whole process group */
 	if (kill(-pid, sig))
@@ -106,18 +142,28 @@ pg_signal_backend(int pid, int sig)
 	return true;
+ * Signal to cancel a backend process.	This is allowed if you are superuser or
+ * have the same role as the process being canceled.
+ */
-	PG_RETURN_BOOL(pg_signal_backend(PG_GETARG_INT32(0), SIGINT));
+	PG_RETURN_BOOL(pg_signal_backend(PG_GETARG_INT32(0), SIGINT, true));
+ * Signal to terminate a backend process.  Only allowed by superuser.
+ */
-	PG_RETURN_BOOL(pg_signal_backend(PG_GETARG_INT32(0), SIGTERM));
+	PG_RETURN_BOOL(pg_signal_backend(PG_GETARG_INT32(0), SIGTERM, false));
+ * Signal to reload the database configuration
+ */
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to