Re: Patch to allow users to kill their own queries

On Fri, Dec 16, 2011 at 13:31, Greg Smith <greg(at)2ndquadrant(dot)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
future..

* 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/