On Fri, Mar 11, 2011 at 8:54 AM, Bruce Momjian <br...@momjian.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
