Re: Patch to allow users to kill their own queries - Mailing list pgsql-hackers

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    greg@2ndQuadrant.com   Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support  www.2ndQuadrant.us