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);
<para>
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.
</para>
<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>
<entry><type>boolean</type></entry>
- <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>
</row>
<row>
<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.
</para>
<para>
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,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- (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,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ (errmsg("must be superuser or have the same role to signal other server processes"))));
+ }
+ else
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ 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 */
#ifdef HAVE_SETSID
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.
+ */
Datum
pg_cancel_backend(PG_FUNCTION_ARGS)
{
- 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.
+ */
Datum
pg_terminate_backend(PG_FUNCTION_ARGS)
{
- 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
+ */
Datum
pg_reload_conf(PG_FUNCTION_ARGS)
{
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
