On Tue, Dec 13, 2011 at 5:59 AM, Greg Smith <g...@2ndquadrant.com> wrote: > Same-user cancels, but not termination. Only this, and nothing more.
+1 from me on this approach. I think enough people have clamored for this simple approach which solves the common-case. > 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 think that's OK, it should be easy enough to reorganize if more callers to pg_signal_backend() happen to come along. The only niggling concern I have about this patch (and, I think, all similar ones proposed) is the possible race condition between the permissions checking and the actual call of kill() inside pg_signal_backend(). I fooled around with this by using gdb to attach to Backend #1, stuck a breakpoint right before the call to: if (kill(-pid, sig)) and ran a pg_cancel_backend() of a same-role Backend #2. Then, with the permissions checking passed, I exited Backend #2 and used a script to call fork() in a loop until my system PIDs had wrapped around to a few PIDs before the one Backend #2 had held. Then, I opened a few superuser connections until I had one with the same PID which Backend #2 previously had. After I released the breakpoint in gdb on Backend #1, it happily sent a SIGINT to my superuser backend. I notice that BackendPidGetProc() has the comment: ... Note that it is up to the caller to be sure that the question remains meaningful for long enough for the answer to be used ... So someone has mulled this over before. It took my script maybe 15-20 minutes to cause a wraparound by running fork() in a loop, and that wraparound would somehow have to occur within the short execution of pg_signal_backend(). I'm not super worried about the patch from a security perspective, just thought I'd point this out. Josh -- Sent via pgsql-hackers mailing list (firstname.lastname@example.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers