The submission from Edward Muller I'm replying to is quite similar to
what the other raging discussion here decided was the right level to
target. There was one last year from Josh Kupershmidt with similar
goals: http://archives.postgresql.org/pgsql-admin/2010-02/msg00052.php
A good place to start is the concise summary of the new specification
goal that Tom made in the other thread:
> If allowing same-user cancels is enough to solve 95% or 99% of the
real-world use cases, let's just do that.
Same-user cancels, but not termination. Only this, and nothing more.
Relative to that goal, Ed's patch was too permissive for termination,
and since he's new to this code it didn't check all the error conditions
possible here. Josh's patch had many of the right error checks, but it
was more code than I liked for his slightly different permissions
change. And its attempts to be helpful leaked role information. (That
may have been just debugging debris left for review purposes) I mashed
the best bits of both together, tried to simplify the result, then
commented heavily upon the race conditions and design decisions the code
reflects. Far as I can tell the patch is feature complete, including
documentation.
Appropriate credits here would go Josh Kupershmidt, Edward Muller, and
then myself; everyone did an equally useful chunk of this in that
order. It's all packaged up for useful gitsumption at
https://github.com/greg2ndQuadrant/postgres/tree/cancel_backend too. I
attached it to the next CommitFest:
https://commitfest.postgresql.org/action/patch_view?id=722 but would
enjoy seeing a stake finally put through its evil heart before then, as
I don't think there's much left to do now.
To demo I start with a limited user and a crazy, must be stopped backend:
$ createuser test
Shall the new role be a superuser? (y/n) n
Shall the new role be allowed to create databases? (y/n) n
Shall the new role be allowed to create more new roles? (y/n) n
$ psql -U test
test=> select pg_sleep(1000000);
Begin another session, find and try to terminate; get rejected with a
hint, then follow it to cancel:
test=> select procpid,usename,current_query from pg_stat_activity;
-[ RECORD 1 ]-+------------------------------------------------------------
procpid | 28154
usename | test
current_query | select pg_sleep(1000000);
test=> select pg_terminate_backend(28154);
ERROR: must be superuser to terminate other server processes
HINT: you can use pg_cancel_backend() on your own processes
test=> select pg_cancel_backend(28154);
-[ RECORD 1 ]-----+--
pg_cancel_backend | t
And then this is shown on the first one:
test=> select pg_sleep(1000000);
ERROR: canceling statement due to user request
Victory over the evil sleeping backend is complete, without a superuser
in sight.
There's one obvious and questionable design decision I made to
highlight. Right now the only consumers of pg_signal_backend are the
cancel and terminate calls. What I did was make pg_signal_backend more
permissive, adding the idea that role equivalence = allowed, and
therefore granting that to anything else that might call it. And then I
put a stricter check on termination. This results in a redundant check
of superuser on the termination check, and the potential for mis-using
pg_signal_backend. I documented all that and liked the result; it feels
better to me to have pg_signal_backend provide an API that is more
flexible here. Pushback to structure this differently is certainly
possible though, and I'm happy to iterate the patch to address that. It
might drift back toward something closer to Josh's original design.
--
Greg Smith 2ndQuadrant US g...@2ndquadrant.com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index e7f7fe0..f145c3f 100644
*** a/doc/src/sgml/func.sgml
--- b/doc/src/sgml/func.sgml
*************** SELECT set_config('log_statement_stats',
*** 14244,14251 ****
<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.
</para>
<table id="functions-admin-signal-table">
--- 14244,14251 ----
<para>
The functions shown in <xref
linkend="functions-admin-signal-table"> send control signals to
! other server processes. Use of these functions is usually restricted
! to superusers, with noted exceptions.
</para>
<table id="functions-admin-signal-table">
*************** SELECT set_config('log_statement_stats',
*** 14262,14268 ****
<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>
</row>
<row>
<entry>
--- 14262,14271 ----
<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. 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>
*************** SELECT set_config('log_statement_stats',
*** 14304,14309 ****
--- 14307,14316 ----
<command>postgres</command> processes on the server (using
<application>ps</> on Unix or the <application>Task
Manager</> on <productname>Windows</>).
+ For the more permissive <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..b052149 100644
*** a/src/backend/utils/adt/misc.c
--- b/src/backend/utils/adt/misc.c
***************
*** 30,35 ****
--- 30,36 ----
#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"
*************** current_query(PG_FUNCTION_ARGS)
*** 71,84 ****
/*
* Functions to send signals to other backends.
*/
static bool
pg_signal_backend(int pid, int sig)
{
! if (!superuser())
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
! (errmsg("must be superuser to signal other server processes"))));
if (!IsBackendPid(pid))
{
--- 72,115 ----
/*
* Functions to send signals to other backends.
+ *
+ * When calling pg_signal_backend, non-superusers are allowed to send signals
+ * to other backends if they are running as the same role. Make sure you're
+ * comfortable with that before using it for other types of signaling. If not,
+ * add your own checks first, as pg_terminate_backend does here.
*/
static bool
pg_signal_backend(int pid, int sig)
{
! PGPROC *proc;
! bool allowed = false;
!
! if (superuser())
! allowed = true;
! else
! {
! /*
! * Check for matching roles if we've already failed the superuser test.
! *
! * 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, and we don't want to acquire a lock just
! * just to eliminate that possibility. Since the signal being passed
! * might be a request for cancellation, this is not necessarily even a
! * problem.
! */
! proc = BackendPidGetProc(pid);
!
! if ((proc != NULL) && (proc->roleId == GetUserId()))
! allowed = true;
! }
!
! if (!allowed)
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
! (errmsg("must be superuser or have the same role to signal other server processes"))));
if (!IsBackendPid(pid))
{
*************** pg_cancel_backend(PG_FUNCTION_ARGS)
*** 115,120 ****
--- 146,161 ----
Datum
pg_terminate_backend(PG_FUNCTION_ARGS)
{
+ /*
+ * Since the permissions check for signaling isn't as strict as for
+ * termination, run the superuser only one here and suggest alternatives.
+ */
+ if (!superuser())
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("must be superuser to terminate other server processes"),
+ errhint("you can use pg_cancel_backend() on your own processes")));
+
PG_RETURN_BOOL(pg_signal_backend(PG_GETARG_INT32(0), SIGTERM));
}
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
