On Mon, Dec 19, 2011 at 15:50, Greg Smith <g...@2ndquadrant.com> wrote:
> On 12/18/2011 07:31 AM, Magnus Hagander wrote:
>> * 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.
> That looks fine.  I highlighted this because I had a feeling there was still
> some gain to be had here, just didn't see it myself.  This works.
>> * 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
>> future..
> I feel that including a hint in the pg_terminate_backend case is a UI
> requirement.  If someone has made it as far as discovering that function
> exists, tries calling it, and it fails, the friendly thing to do is point
> them toward a direction that might work better.  Little things like that
> make a huge difference in how friendly the software appears to its users;
> this is even more true in cases where version improvements actually expand
> what can and cannot be done.
> My quick and dirty side thinks that just documenting the potential future
> issues would be enough:
> "Due to the limited number of callers of this function, the hint message
> here can be certain that pg_terminate_backend provides the only path to
> reach this point.  If more callers to pg_signal_backend appear, a more
> generic hint mechanism might be needed here."
> If you must have this more generic mechanism available, I would accept
> re-factoring to provide it instead.  What I wouldn't want to live with is a
> commit of this where the hint goes away completely.  It's taken a long time
> chopping the specification to get this feature sorted out; we might as well
> make what's left be the best it can be now.

How about something like this - passing it in as a parameter?

That said - can someone who knows the translation stuff better than me
comment on if this is actually going to be translatable, or if it
violates too many translation rules?

 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..45520b6 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,45 @@ current_query(PG_FUNCTION_ARGS)
- * Functions to send signals to other backends.
+ * Send a signal to another backend.
+ * If allow_same_role is false, actionstr must be set to a string
+ * indicating what the signal does, to be inserted in the error message, and
+ * hint should be set to a hint to be sent along with this message.
 static bool
-pg_signal_backend(int pid, int sig)
+pg_signal_backend(int pid, int sig, bool allow_same_role, const char *actionstr, const char *hint)
+	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 %s other server processes", actionstr),
+					 errhint("%s", hint)));
+	}
 	if (!IsBackendPid(pid))
@@ -91,6 +122,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 +146,30 @@ 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, NULL, NULL));
+ * 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,
+									 gettext_noop("terminate"),
+									 gettext_noop("You can cancel your own processes with pg_cancel_backend().")));
+ * 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