Re: 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(at)postgresql(dot)org just spins for me.

Edward Muller <edward(at)heroku(dot)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(dot)Grittner(at)wicourts(dot)gov> wrote:
> Edward Muller <edward(at)heroku(dot)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
>

On Wed, Nov 16, 2011 at 12:06 PM, Kevin Grittner
<Kevin(dot)Grittner(at)wicourts(dot)gov> wrote:
> Edward Muller <edward(at)heroku(dot)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(dot)Grittner(at)wicourts(dot)gov> wrote:
> Edward Muller <edward(at)heroku(dot)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(at)2ndQuadrant(dot)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(at)2ndQuadrant(dot)com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us

On Tue, Dec 13, 2011 at 11:59, Greg Smith <greg(at)2ndquadrant(dot)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(at)2ndQuadrant(dot)com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> On Tue, Dec 13, 2011 at 11:59, Greg Smith <greg(at)2ndquadrant(dot)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(at)2ndquadrant(dot)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

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(at)2ndQuadrant(dot)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(at)2ndQuadrant(dot)com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us

On Fri, Dec 16, 2011 at 1:21 AM, Greg Smith <greg(at)2ndquadrant(dot)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(at)2ndQuadrant(dot)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(at)2ndquadrant(dot)com<javascript:;>>
> 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(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Fri, Dec 16, 2011 at 1:21 AM, Greg Smith <greg(at)2ndquadrant(dot)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(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/

On Sat, Dec 17, 2011 at 11:58 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)2ndQuadrant(dot)com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us

On Mon, Dec 19, 2011 at 15:50, Greg Smith <greg(at)2ndquadrant(dot)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/

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(at)leadboat(dot)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(at)leadboat(dot)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(at)2ndQuadrant(dot)com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.com

On Fri, Jan 13, 2012 at 14:42, Greg Smith <greg(at)2ndquadrant(dot)com> wrote:
> On 01/03/2012 12:59 PM, Tom Lane wrote:
>>
>> Noah Misch<noah(at)leadboat(dot)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(at)hagander(dot)net> wrote:
> On Fri, Jan 13, 2012 at 14:42, Greg Smith <greg(at)2ndquadrant(dot)com> wrote:
>> On 01/03/2012 12:59 PM, Tom Lane wrote:
>>>
>>> Noah Misch<noah(at)leadboat(dot)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/