Thread: Patch to allow users to kill their own queries
Looking for comments ... https://gist.github.com/be937d3a7a5323c73b6e We'd like to get this, or something like it, into 9.2 PS Subscribing to psql-hackers@postgresql.org just spins for me.
Edward Muller <edward@heroku.com> wrote: > Looking for comments ... > > https://gist.github.com/be937d3a7a5323c73b6e > > We'd like to get this, or something like it, into 9.2 If you want it to be seriously considered, you should post the patch to this list, which makes it part of the permanent archives and indicates your willingness to place the code under the PostgreSQL license. http://wiki.postgresql.org/wiki/Submitting_a_Patch#Patch_submission Unfortunately, you're a day late in terms of getting it reviewed in the just-started CommitFest, but another will start in two months. https://commitfest.postgresql.org/action/commitfest_view/open Of course, you're free to talk about it until then, and perhaps someone will take a look at it before the next CF; but a lot of attention will be focused on the patches submitted by the start of the current CF. Any chance you could help review patches in the CF in progress, to help get others' time freed up sooner?: https://commitfest.postgresql.org/action/commitfest_view/inprogress It's a good way to become familiar with the process, so that you can better move your own patch along. -Kevin
On Wed, Nov 16, 2011 at 12:06 PM, Kevin Grittner <Kevin.Grittner@wicourts.gov> wrote: > Edward Muller <edward@heroku.com> wrote: > >> Looking for comments ... >> >> https://gist.github.com/be937d3a7a5323c73b6e >> >> We'd like to get this, or something like it, into 9.2 > > If you want it to be seriously considered, you should post the patch > to this list, which makes it part of the permanent archives and > indicates your willingness to place the code under the PostgreSQL > license. inline .... > > http://wiki.postgresql.org/wiki/Submitting_a_Patch#Patch_submission I think this was done right. > [snip] > > Any chance you could help review patches in the CF in progress, to > help get others' time freed up sooner?: > > https://commitfest.postgresql.org/action/commitfest_view/inprogress Well, I'm totally new to the code base, I don't write in *C* very often (probably obvious from my patch). Maybe over time.... > > It's a good way to become familiar with the process, so that you can > better move your own patch along. > > -Kevin >
Attachment
On Wed, Nov 16, 2011 at 12:06 PM, Kevin Grittner <Kevin.Grittner@wicourts.gov> wrote: > Edward Muller <edward@heroku.com> wrote: > >> Looking for comments ... >> >> https://gist.github.com/be937d3a7a5323c73b6e >> >> We'd like to get this, or something like it, into 9.2 On Wed, Nov 16, 2011 at 12:06 PM, Kevin Grittner <Kevin.Grittner@wicourts.gov> wrote: > Edward Muller <edward@heroku.com> wrote: > >> Looking for comments ... >> >> https://gist.github.com/be937d3a7a5323c73b6e >> >> We'd like to get this, or something like it, into 9.2 As it would turn out, a patch for this has already been submitted: http://archives.postgresql.org/pgsql-hackers/2011-10/msg00001.php There was some wrangling on whether it needs to be extended to be useful, but for our purposes the formulation already posted already captures vital value for us, and in that form appears to be fairly uncontentious. I have moved it to the current commitfest, with a comment linking to the 'please revive this patch' thread whereby a second glance at what to do about this was conducted. The link follows: https://commitfest.postgresql.org/action/patch_view?id=541 > If you want it to be seriously considered, you should post the patch > to this list, which makes it part of the permanent archives and > indicates your willingness to place the code under the PostgreSQL > license. Although technical mailing lists are not primarily a place of reflection and sensitivity, I do think that wording addressed to a new participant could have been kinder. Perhaps, "Unfortunately we cannot accept or even read your patch because of licensing concerns, would you please follow the following patch submission guidelines?" <link>. -- fdr
On 11/16/2011 01:28 PM, Daniel Farina wrote: > As it would turn out, a patch for this has already been submitted: > http://archives.postgresql.org/pgsql-hackers/2011-10/msg00001.php > > There was some wrangling on whether it needs to be extended to be > useful, but for our purposes the formulation already posted already > captures vital value for us, and in that form appears to be fairly > uncontentious. I have moved it to the current commitfest, with a > comment linking to the 'please revive this patch' thread whereby a > second glance at what to do about this was conducted. Yeah, that one got a raw deal; it should be in the *current* CommitFest--you had it in the next one. I'll join the chorus to just allow people to fire just the pg_cancel_backend pea shooter foot gun targeted only at their own feet, make most users happy, and punt anything more complicated off as troublesome relative to its benefit. That's what Daniel has said, what his co-worker Edward also implemented, what Noah thought was good enough given other security mechanisms in place, and what Tom thinks is reasonable too. This I feel is important, so I'm going to add myself as the next reviewer and include it in my testing run tomorrow. -- Greg Smith 2ndQuadrant US greg@2ndQuadrant.com Baltimore, MD PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us
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
Attachment
On Tue, Dec 13, 2011 at 11:59, Greg Smith <greg@2ndquadrant.com> wrote: > 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. Good. > 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. > > 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 That HINT sounds a bit weird to me. "you can cancel your own queries using pg_cancel_backend() instead" or something like that? > Victory over the evil sleeping backend is complete, without a superuser in > sight. \o/ > 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. How about passing a parameter to pg_signal_backend? Making pg_signal_backend(int pid, int sig, bool allow_samerole)? -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
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 sounds like it will result in less code, and make the API I was documenting be obvious from the parameters instead. I'll refactor to do that, tweak the hint message, and resubmit. -- Greg Smith 2ndQuadrant US greg@2ndQuadrant.com Baltimore, MD PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us
Magnus Hagander <magnus@hagander.net> writes: > On Tue, Dec 13, 2011 at 11:59, Greg Smith <greg@2ndquadrant.com> wrote: >> HINT: you can use pg_cancel_backend() on your own processes > That HINT sounds a bit weird to me. "you can cancel your own queries > using pg_cancel_backend() instead" or something like that? Doesn't follow message style guidelines either ;-). Hints should be complete sentences, with initial cap and ending period. http://www.postgresql.org/docs/devel/static/error-style-guide.html regards, tom lane
On Tue, Dec 13, 2011 at 5:59 AM, Greg Smith <greg@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 tobe 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
On 12/15/2011 07:36 PM, Josh Kupershmidt wrote: > 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(). This is a problem with the existing code though, and the proposed changes don't materially alter that; there's just another quick check in one path through. Right now we check if someone is superuser, then if it's a backend PID, then we send the signal. If you assume someone can run through all the PIDs between those checks and the kill, the system is already broken that way. > 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(). Right, the the possibility you're raising is the obvious flaw with this approach. Currently the only consumers of BackendPidGetProc or IsBackendPid are these cancel/terminate functions. As you measured, running through the PIDs fast enough to outrace any of that code is unlikely. I'm sure a lot of other UNIX apps would break, too, if that ever became untrue. The sort of thing that comment is warning is that you wouldn't want to do something like grab a PID, vacuum a table, and then assume that PID was still valid. -- Greg Smith 2ndQuadrant US greg@2ndQuadrant.com Baltimore, MD PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us
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. -- Greg Smith 2ndQuadrant US greg@2ndQuadrant.com Baltimore, MD PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us
Attachment
On Fri, Dec 16, 2011 at 1:21 AM, Greg Smith <greg@2ndquadrant.com> wrote: > This is a problem with the existing code though, and the proposed changes > don't materially alter that; there's just another quick check in one path > through. Right now we check if someone is superuser, then if it's a backend > PID, then we send the signal. If you assume someone can run through all the > PIDs between those checks and the kill, the system is already broken that > way. From a theoretical point of view, I believe it to be slightly different. If a superuser sends a kill, they will certainly be authorized to kill whatever they end up killing, because they are authorized to kill anything. On the other hand, the proposed patch would potentially result - in the extremely unlikely event of a super-fast PID wraparound - in someone cancelling a query they otherwise wouldn't have been able to cancel. In practice, the chances of this seem fairly remote. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 12/16/2011 08:42 AM, Robert Haas wrote: > the proposed patch would potentially result - in the extremely > unlikely event of a > super-fast PID wraparound - in someone cancelling a query they > otherwise wouldn't have been able to cancel. > So how might this get exploited? -Attach a debugger and put a breakpoint between the check and the kill -Fork processes to get close to your target -Wait for a process you want to mess with to appear at the PID you're waiting for. If you miss it, repeat fork bomb and try again. -Resume the debugger to kill the other user's process If I had enough access to launch this sort of attack, I think I'd find mayhem elsewhere more a more profitable effort. Crazy queries, work_mem abuse, massive temp file generation, trying to get the OOM killer involved; seems like there's bigger holes than this already. -- Greg Smith 2ndQuadrant US greg@2ndQuadrant.com Baltimore, MD PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us
On Friday, December 16, 2011, Robert Haas wrote:
On Fri, Dec 16, 2011 at 1:21 AM, Greg Smith <greg@2ndquadrant.com> wrote:
> This is a problem with the existing code though, and the proposed changes
> don't materially alter that; there's just another quick check in one path
> through. Right now we check if someone is superuser, then if it's a backend
> PID, then we send the signal. If you assume someone can run through all the
> PIDs between those checks and the kill, the system is already broken that
> way.
>From a theoretical point of view, I believe it to be slightly
different. If a superuser sends a kill, they will certainly be
authorized to kill whatever they end up killing, because they are
authorized to kill anything. On the other hand, the proposed patch
Not necessarily. What if it's recycled as a backend in a different postgres installation. Or just a cronjob or shell running as the same user?
Sure, you can argue that the superuser can destroy anything he wants - but in that case, why do we have a check for this at all in the first place?
I think we can safely say that any OS that actually manages to recycle the PID in the short time it takes to get between those instructions is so broken we don't need to care about that.
On Friday, December 16, 2011, Greg Smith wrote:
On 12/16/2011 08:42 AM, Robert Haas wrote:the proposed patch would potentially result - in the extremely unlikely event of a
super-fast PID wraparound - in someone cancelling a query they
otherwise wouldn't have been able to cancel.
So how might this get exploited?
-Attach a debugger and put a breakpoint between the check and the kill
Once you've attached a debugger, you've already won. You can inject arbitrary instructions at this point, no?
-Fork processes to get close to your target
-Wait for a process you want to mess with to appear at the PID you're waiting for. If you miss it, repeat fork bomb and try again.
-Resume the debugger to kill the other user's process
If I had enough access to launch this sort of attack, I think I'd find mayhem elsewhere more a more profitable effort. Crazy queries, work_mem abuse, massive temp file generation, trying to get the OOM killer involved; seems like there's bigger holes than this already.
"killall -9 postgres" is even easier.
//Magnus
Excerpts from Greg Smith's message of vie dic 16 11:19:52 -0300 2011: > On 12/16/2011 08:42 AM, Robert Haas wrote: > > the proposed patch would potentially result - in the extremely > > unlikely event of a > > super-fast PID wraparound - in someone cancelling a query they > > otherwise wouldn't have been able to cancel. > > > > So how might this get exploited? > > -Attach a debugger and put a breakpoint between the check and the kill If you can attach a debugger to a backend, you already have the credentials to the user running postmaster, which we assume to be a "game over" condition. I guess a more interesting exploitation would be to do this until the killing backend randomly wins the race condition against a dying backend and a new one starting up; this would require very rapid reuse of the PID, plus very rapid startup of the new proces. I think we already assume that this is not the case in other code paths, even on systems that randomly (instead of sequentially) assign PIDs. -- Álvaro Herrera <alvherre@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, Dec 16, 2011 at 1:21 AM, Greg Smith <greg@2ndquadrant.com> wrote:
>> ... If you assume someone can run through all the
>> PIDs between those checks and the kill, the system is already broken that
>> way.
> From a theoretical point of view, I believe it to be slightly
> different. If a superuser sends a kill, they will certainly be
> authorized to kill whatever they end up killing, because they are
> authorized to kill anything. On the other hand, the proposed patch
> would potentially result - in the extremely unlikely event of a
> super-fast PID wraparound - in someone cancelling a query they
> otherwise wouldn't have been able to cancel.
> In practice, the chances of this seem fairly remote.
I think this argument is bogus: if this is a real issue, then no use of
kill() anytime, by anyone, is safe. In practice I believe that Unix
systems avoid recycling PIDs right away so as to offer some protection.
regards, tom lane
On Fri, Dec 16, 2011 at 13:31, Greg Smith <greg@2ndquadrant.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/
Attachment
On Sat, Dec 17, 2011 at 11:58 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > I think this argument is bogus: if this is a real issue, then no use of > kill() anytime, by anyone, is safe. In practice I believe that Unix > systems avoid recycling PIDs right away so as to offer some protection. I'm not sure they do anything more sophisticated than cycling through a sufficiently-large PID space, but whether it's that or something else, I guess it must be adequate or they'd have enlarged the space... -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 12/18/2011 07:31 AM, Magnus Hagander wrote: > * 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. That looks fine. I highlighted this because I had a feeling there was still some gain to be had here, just didn't see it myself. This works. > * 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 feel that including a hint in the pg_terminate_backend case is a UI requirement. If someone has made it as far as discovering that function exists, tries calling it, and it fails, the friendly thing to do is point them toward a direction that might work better. Little things like that make a huge difference in how friendly the software appears to its users; this is even more true in cases where version improvements actually expand what can and cannot be done. My quick and dirty side thinks that just documenting the potential future issues would be enough: "Due to the limited number of callers of this function, the hint message here can be certain that pg_terminate_backend provides the only path to reach this point. If more callers to pg_signal_backend appear, a more generic hint mechanism might be needed here." If you must have this more generic mechanism available, I would accept re-factoring to provide it instead. What I wouldn't want to live with is a commit of this where the hint goes away completely. It's taken a long time chopping the specification to get this feature sorted out; we might as well make what's left be the best it can be now. -- Greg Smith 2ndQuadrant US greg@2ndQuadrant.com Baltimore, MD PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us
On Mon, Dec 19, 2011 at 15:50, Greg Smith <greg@2ndquadrant.com> wrote: > On 12/18/2011 07:31 AM, Magnus Hagander wrote: >> >> * 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. > > > That looks fine. I highlighted this because I had a feeling there was still > some gain to be had here, just didn't see it myself. This works. > > >> * 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 feel that including a hint in the pg_terminate_backend case is a UI > requirement. If someone has made it as far as discovering that function > exists, tries calling it, and it fails, the friendly thing to do is point > them toward a direction that might work better. Little things like that > make a huge difference in how friendly the software appears to its users; > this is even more true in cases where version improvements actually expand > what can and cannot be done. > > My quick and dirty side thinks that just documenting the potential future > issues would be enough: > > "Due to the limited number of callers of this function, the hint message > here can be certain that pg_terminate_backend provides the only path to > reach this point. If more callers to pg_signal_backend appear, a more > generic hint mechanism might be needed here." > > If you must have this more generic mechanism available, I would accept > re-factoring to provide it instead. What I wouldn't want to live with is a > commit of this where the hint goes away completely. It's taken a long time > chopping the specification to get this feature sorted out; we might as well > make what's left be the best it can be now. How about something like this - passing it in as a parameter? That said - can someone who knows the translation stuff better than me comment on if this is actually going to be translatable, or if it violates too many translation rules? -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Attachment
On Tue, Dec 20, 2011 at 02:30:08PM +0100, Magnus Hagander wrote:
> That said - can someone who knows the translation stuff better than me
> comment on if this is actually going to be translatable, or if it
> violates too many translation rules?
> +pg_signal_backend(int pid, int sig, bool allow_same_role, const char *actionstr, const char *hint)
> + ereport(ERROR,
> + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> + errmsg("must be superuser to %s other server processes", actionstr),
> + errhint("%s", hint)));
> + PG_RETURN_BOOL(pg_signal_backend(PG_GETARG_INT32(0), SIGTERM, false,
> + gettext_noop("terminate"),
> + gettext_noop("You can cancel your own processes with pg_cancel_backend().")));
> }
You need "errhint("%s", _(hint))" or "errhint(hint)" to substitute the
translation at runtime; only the printf-pattern string gets an automatic
message catalog lookup.
Regarding the other message, avoid composing a translated message from
independently-translated parts. The translator sees this:
#: utils/adt/misc.c:110
#, c-format
msgid "must be superuser to %s other server processes"
msgstr ""
#: utils/adt/misc.c:166
msgid "terminate"
msgstr ""
#: utils/adt/misc.c:167
msgid "You can cancel your own processes with pg_cancel_backend()."
msgstr ""
He'll probably need to read the code to see how the strings go together. If
we add an alternative to "terminate", not all languages will necessarily have
a translation of the outer message that works for both inner fragments.
Noah Misch <noah@leadboat.com> writes:
> Regarding the other message, avoid composing a translated message from
> independently-translated parts.
Yes. I haven't looked at the patch, but I wonder whether it wouldn't be
better to dodge both of these problems by having the subroutine return a
success/failure result code, so that the actual ereport can be done at
an outer level where the full message (and hint) can be written
directly.
regards, tom lane
On 01/03/2012 12:59 PM, Tom Lane wrote: > Noah Misch<noah@leadboat.com> writes: > >> Regarding the other message, avoid composing a translated message from >> independently-translated parts. >> > Yes. I haven't looked at the patch, but I wonder whether it wouldn't be > better to dodge both of these problems by having the subroutine return a > success/failure result code, so that the actual ereport can be done at > an outer level where the full message (and hint) can be written > directly. > Between your and Noah's comments I understand the issue and likely direction to sort this out now. Bounced forward to the next CommitFest and back on my plate again. Guess I'll be learning new and exciting things about translation this week. -- Greg Smith 2ndQuadrant US greg@2ndQuadrant.com Baltimore, MD PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.com
On Fri, Jan 13, 2012 at 14:42, Greg Smith <greg@2ndquadrant.com> wrote: > On 01/03/2012 12:59 PM, Tom Lane wrote: >> >> Noah Misch<noah@leadboat.com> writes: >> >>> >>> Regarding the other message, avoid composing a translated message from >>> independently-translated parts. >>> >> >> Yes. I haven't looked at the patch, but I wonder whether it wouldn't be >> better to dodge both of these problems by having the subroutine return a >> success/failure result code, so that the actual ereport can be done at >> an outer level where the full message (and hint) can be written >> directly. >> > > > Between your and Noah's comments I understand the issue and likely direction > to sort this out now. Bounced forward to the next CommitFest and back on my > plate again. Guess I'll be learning new and exciting things about > translation this week. I should say that I have more or less finished this one off, since I figured you were working on more important things. Just a final round of cleanup and commit left, really, so you can take it off your plate again if you want to :-) -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
On Fri, Jan 13, 2012 at 14:46, Magnus Hagander <magnus@hagander.net> wrote: > On Fri, Jan 13, 2012 at 14:42, Greg Smith <greg@2ndquadrant.com> wrote: >> On 01/03/2012 12:59 PM, Tom Lane wrote: >>> >>> Noah Misch<noah@leadboat.com> writes: >>> >>>> >>>> Regarding the other message, avoid composing a translated message from >>>> independently-translated parts. >>>> >>> >>> Yes. I haven't looked at the patch, but I wonder whether it wouldn't be >>> better to dodge both of these problems by having the subroutine return a >>> success/failure result code, so that the actual ereport can be done at >>> an outer level where the full message (and hint) can be written >>> directly. >>> >> >> >> Between your and Noah's comments I understand the issue and likely direction >> to sort this out now. Bounced forward to the next CommitFest and back on my >> plate again. Guess I'll be learning new and exciting things about >> translation this week. > > I should say that I have more or less finished this one off, since I > figured you were working on more important things. Just a final round > of cleanup and commit left, really, so you can take it off your plate > again if you want to :-) Finally, to end the bikeshedding, I've applied this patch after another round of fairly extensive changes. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/