Re: pg_terminate_backend and pg_cancel_backend by not administrator user

On Fri, Mar 11, 2011 at 8:54 AM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> I have added it to the next commit fest.

Hi Torello,

I have volunteered (more accurately, Greg Smith "volunteered" me :-)
to be a reviewer for this patch. I know you're a bit new here, so I
thought I'd outline where this patch stands and what's expected if
you'd like to move it along.

We organize patch reviews via "commitfests" lasting a month or so.
Some more information about this process:
http://wiki.postgresql.org/wiki/CommitFest

Each commitfest is a period wherein you can expect to receive some
feedback on your patch and advice on things which might need to be
improved (in this case, it's my job to provide you this feedback).
Your patch is in the upcoming commitfest, scheduled to run from June
15 to July 14.

So if you're interested in being responsible for this patch, or some
variant of it, eventually making its way into PostgreSQL 9.2, you
should be willing to update your patch based on feedback, request
advice, etc. during this period. If you're not interested in getting
sucked into this process that's OK -- just please advise us if that's
the case, and maybe someone else will be willing to take charge of the
patch.

Anssi and I posted some initial feedback on the patch's goals earlier.
I would like to ultimately see users have the capability to
pg_cancel_backend() their own queries. But I could at least conceive
of others not wanting this behavior enabled by default. So perhaps
this patch's approach of granting extra privs to the database owner
could work as a first attempt. And maybe a later version could
introduce a GUC allowing the DBA to control whether users can
cancel/terminate their backends, or we could instead have an option
flag to CREATE/ALTER ROLE, allowing per-user configuration.

It would be helpful to hear from others whether this patch's goals
would work as a first pass at this problem, so that Torello doesn't
waste time on a doomed approach. Also, it might be helpful to add an
entry on the Todo list for 'allow non-superusers to use
pg_cancel_backend()', in case this patch gets sunk.

Now, a few technical comments about the patch:
1.) This bit looks dangerous:
+ backend = pgstat_fetch_stat_beentry(i);
+ if (backend->st_procpid == pid) {

Since pgstat_fetch_stat_beentry() might return NULL.

I'm a bit suspicious about whether looping through
pgstat_fetch_stat_beentry() is the best way to determine the database
owner for a given backend PID, but I haven't dug in enough yet to
suggest a better alternative.

2.) The way the code inside pg_signal_backend() is structured, doing:
select pg_cancel_backend(12345);

as non-superuser, where '12345' is a fictitious PID, can now give you
the incorrect error message:

ERROR: must be superuser or target database owner to signal other
server processes

3.) No documentation adjustments, and the comments need some cleaup.
Torello: I'll be happy to handle comments/documentation for you as a
native English speaker, so you don't have to worry about this part.

That's it for now. Torello, I look forward to hearing back from you,
and hope that you have some time to work on this patch further.

Josh