Re: pg_terminate_backend for same-role

Parallel to pg_cancel_backend, it'd be nice to allow the user to just
outright kill a backend that they own (politely, with a SIGTERM),
aborting any transactions in progress, including the idle transaction,
and closing the socket.

I imagine the problem is a race condition whereby a pid might be
reused by another process owned by another user (doesn't that also
affect pg_cancel_backend?). Shall we just do everything using the
MyCancelKey (which I think could just be called "SessionKey",
"SessionSecret", or even just "Session") as to ensure we have no case
of mistaken identity? Or does that end up being problematic?

--
fdr

On Fri, Mar 16, 2012 at 8:14 AM, Daniel Farina <daniel(at)heroku(dot)com> wrote:
> Parallel to pg_cancel_backend, it'd be nice to allow the user to just
> outright kill a backend that they own (politely, with a SIGTERM),
> aborting any transactions in progress, including the idle transaction,
> and closing the socket.

+1

> I imagine the problem is a race condition whereby a pid might be
> reused by another process owned by another user (doesn't that also
> affect pg_cancel_backend?).

Yes, but I think it's too unlikely to happen. Not sure if we really
should worry about that.

> Shall we just do everything using the
> MyCancelKey (which I think could just be called "SessionKey",
> "SessionSecret", or even just "Session") as to ensure we have no case
> of mistaken identity? Or does that end up being problematic?

What if pid is unfortunately reused after passing the test of MyCancelKey
and before sending the signal?

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On Thu, Mar 15, 2012 at 9:39 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>> Shall we just do everything using the
>> MyCancelKey (which I think could just be called "SessionKey",
>> "SessionSecret", or even just "Session") as to ensure we have no case
>> of mistaken identity? Or does that end up being problematic?
>
> What if pid is unfortunately reused after passing the test of MyCancelKey
> and before sending the signal?

The way MyCancelKey is checked now is backwards, in my mind. It seems
like it would be better checked by the receiving PID (one can use a
check/recheck also, if so inclined). Is there a large caveat to that?
I'm working on a small patch to do this I hope to post soon (modulus
difficulties), but am fully aware that messing around PGPROC and
signal handling can be a bit fiddly.

--
fdr

Daniel Farina <daniel(at)heroku(dot)com> writes:
> The way MyCancelKey is checked now is backwards, in my mind. It seems
> like it would be better checked by the receiving PID (one can use a
> check/recheck also, if so inclined). Is there a large caveat to that?

You mean, other than the fact that kill(2) can't transmit such a key?

But actually I don't see what you hope to gain from such a change,
even if it can be made to work. Anyone who can do kill(SIGINT) can
do kill(SIGKILL), say --- so you have to be able to trust the signal
sender. What's the point of not trusting it to verify the client
identity?

regards, tom lane

On Thu, Mar 15, 2012 at 10:33 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Daniel Farina <daniel(at)heroku(dot)com> writes:
>> The way MyCancelKey is checked now is backwards, in my mind.  It seems
>> like it would be better checked by the receiving PID (one can use a
>> check/recheck also, if so inclined).  Is there a large caveat to that?
>
> You mean, other than the fact that kill(2) can't transmit such a key?

I was planning on using an out-of-line mechanism. Bad idea?

> But actually I don't see what you hope to gain from such a change,
> even if it can be made to work.  Anyone who can do kill(SIGINT) can
> do kill(SIGKILL), say --- so you have to be able to trust the signal
> sender.  What's the point of not trusting it to verify the client
> identity?

No longer true with pg_cancel_backend not-by-superuser, no? Now there
are new people who can do kill(SIGINT) (modulus the already existing
cancel requests).

--
fdr

Daniel Farina <daniel(at)heroku(dot)com> writes:
> On Thu, Mar 15, 2012 at 10:33 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> But actually I don't see what you hope to gain from such a change,
>> even if it can be made to work. Anyone who can do kill(SIGINT) can
>> do kill(SIGKILL), say --- so you have to be able to trust the signal
>> sender. What's the point of not trusting it to verify the client
>> identity?

> No longer true with pg_cancel_backend not-by-superuser, no?

No. That doesn't affect the above argument in the least. And in fact
if there's any question whatsoever as to whether unprivileged
cross-backend signals are secure, they are not going in in the first
place.

regards, tom lane

On Thu, Mar 15, 2012 at 10:45 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Daniel Farina <daniel(at)heroku(dot)com> writes:
>> On Thu, Mar 15, 2012 at 10:33 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> But actually I don't see what you hope to gain from such a change,
>>> even if it can be made to work.  Anyone who can do kill(SIGINT) can
>>> do kill(SIGKILL), say --- so you have to be able to trust the signal
>>> sender.  What's the point of not trusting it to verify the client
>>> identity?
>
>> No longer true with pg_cancel_backend not-by-superuser, no?
>
> No.  That doesn't affect the above argument in the least.  And in fact
> if there's any question whatsoever as to whether unprivileged
> cross-backend signals are secure, they are not going in in the first
> place.

Okay, well, I believe there is a race in handling common
administrative signals that *might* possibly matter. In the past,
pg_cancel_backend was superuser only, which is a lot like saying "only
available to people who can be the postgres user and run kill." The
cancellation packet is handled via first checking the other backend's
BackendKey and then SIGINTing it, leaving only the most narrow
possibility for a misdirected SIGINT.

But it really is unfortunate that I, a user, run a query or have a
problematic connection of my own role and just want the thing to stop,
but I can't do anything about it without superuser. In recognition of
that, pg_cancel_backend now can operate on backends owned by the same
user (once again, checked before the signal is processed by the
receiver, just like with the cancellation packet).

There was some angst (but I'm not sure about how specific or uniform)
to extend such signaling power to pg_terminate_backend, and the only
objection I can think of is there is this race, or so it would seem to
me. Maybe it's too small to care, in which case we can just extend
the same policy to pg_terminate_backend, or maybe it's not, in which
case we could get rid of any signaling race conditions.

The only hypothetical person who would be happy with the current
situation, if characterized correctly, would be one who thinks that
pid-race on SIGINT from non-superusers (long has been true in the form
of the cancellation packet) is okay but on SIGTERM such a race is not.

--
fdr

On Thu, Mar 15, 2012 at 11:01 PM, Daniel Farina <daniel(at)heroku(dot)com> wrote:
> Okay, well, I believe there is a race in handling common
> administrative signals that *might* possibly matter.  In the past,
> pg_cancel_backend was superuser only, which is a lot like saying "only
> available to people who can be the postgres user and run kill."  The
> cancellation packet is handled via first checking the other backend's
> BackendKey and then SIGINTing it, leaving only the most narrow
> possibility for a misdirected SIGINT.

Attached is a patch that sketches a removal of the caveat by relying
on the BackendId in PGPROC instead of the pid. Basically, the idea is
to make it work more like how cancellation keys work, except for
internal SQL functions. I think the unsatisfying crux here is the
uniqueness of BackendId over the life of one *postmaster* invocation:

sinvaladt.c

MyBackendId = (stateP - &segP->procState[0]) + 1;
/* Advertise assigned backend ID in MyProc */
MyProc->backendId = MyBackendId;

I'm not sure precisely what to think about how this numbering winds up
working on quick inspection. Clearly, if BackendIds can be reused
quickly then the pid-race problem comes back in spirit right away.

But given the contract of MyBackendId as I understand it (it just has
to be unique among all backends at any given time), it could be
changed. I don't *think* it's used for its arithmetic relationship to
its underlying components anywhere.

Another similar solution (not attached) would be to send information
about the originating backend through PGPROC and having the check be
against those rather than merely a correct and unambiguous
MyBackendId.

I also see now that cancellation packets does not have this caveat
because the postmaster is control of all forking and joining in a
serially executed path, so it need not worry about pid racing.

Another nice use for this might be to, say, deliver the memory or
performance stats of another process while-in-execution, without
having to be superuser or and/or gdbing in back to the calling backend.

--
fdr

On Thu, Mar 15, 2012 at 04:14:03PM -0700, Daniel Farina wrote:
> Parallel to pg_cancel_backend, it'd be nice to allow the user to just
> outright kill a backend that they own (politely, with a SIGTERM),
> aborting any transactions in progress, including the idle transaction,
> and closing the socket.

+1

> I imagine the problem is a race condition whereby a pid might be
> reused by another process owned by another user (doesn't that also
> affect pg_cancel_backend?). Shall we just do everything using the
> MyCancelKey (which I think could just be called "SessionKey",
> "SessionSecret", or even just "Session") as to ensure we have no case
> of mistaken identity? Or does that end up being problematic?

No, I think the hazard you identify here is orthogonal to the question of when
to authorize pg_terminate_backend(). As you note downthread, protocol-level
cancellations available in released versions already exhibit this hazard. I
wouldn't mind a clean fix for this, but it's an independent subject.

Here I discussed a hazard specific to allowing pg_terminate_backend():
http://archives.postgresql.org/message-id/20110602045955.GC8246@tornado.gateway.2wire.net

To summarize, user code can trap SIGINT cancellations, but it cannot trap
SIGTERM terminations. If a backend is executing a SECURITY DEFINER function
when another backend of the same role calls pg_terminate_backend() thereon,
the pg_terminate_backend() caller could achieve something he cannot achieve in
PostgreSQL 9.1. I vote that this is an acceptable loss.

Thanks,
nm

On Fri, Mar 16, 2012 at 3:42 PM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> On Thu, Mar 15, 2012 at 04:14:03PM -0700, Daniel Farina wrote:
>> Parallel to pg_cancel_backend, it'd be nice to allow the user to just
>> outright kill a backend that they own (politely, with a SIGTERM),
>> aborting any transactions in progress, including the idle transaction,
>> and closing the socket.
>
> +1
>
>> I imagine the problem is a race condition whereby a pid might be
>> reused by another process owned by another user (doesn't that also
>> affect pg_cancel_backend?).  Shall we just do everything using the
>> MyCancelKey (which I think could just be called "SessionKey",
>> "SessionSecret", or even just "Session") as to ensure we have no case
>> of mistaken identity? Or does that end up being problematic?
>
> No, I think the hazard you identify here is orthogonal to the question of when
> to authorize pg_terminate_backend().  As you note downthread, protocol-level
> cancellations available in released versions already exhibit this hazard.  I
> wouldn't mind a clean fix for this, but it's an independent subject.

Hmm. Well, here's a patch that implements exactly that, I think,
similar to the one posted to this thread, but not using BackendIds,
but rather the newly-introduced "SessionId". Would appreciate
comments. Because an out-of-band signaling method has been integrated
more complex behaviors -- such as closing the
TERM-against-SECURITY-DEFINER-FUNCTION hazard -- can be addressed.
For now I've only attempted to solve the problem of backend ambiguity,
which basically necessitated out-of-line information transfer as per
the usual means.

> Here I discussed a hazard specific to allowing pg_terminate_backend():
> http://archives.postgresql.org/message-id/20110602045955.GC8246@tornado.gateway.2wire.net
>
> To summarize, user code can trap SIGINT cancellations, but it cannot trap
> SIGTERM terminations.  If a backend is executing a SECURITY DEFINER function
> when another backend of the same role calls pg_terminate_backend() thereon,
> the pg_terminate_backend() caller could achieve something he cannot achieve in
> PostgreSQL 9.1.  I vote that this is an acceptable loss.

I'll throw out a patch that just lets this hazard exist and see what
happens, although it is obsoleted/incompatible with the one already
attached.

--
fdr

On Fri, Mar 16, 2012 at 4:42 PM, Daniel Farina <daniel(at)heroku(dot)com> wrote:
> Hmm. Well, here's a patch that implements exactly that, I think,

That version had some screws loose due to some editor snafus.
Hopefully all better.

--
fdr

On Fri, Mar 16, 2012 at 04:42:07PM -0700, Daniel Farina wrote:
> On Fri, Mar 16, 2012 at 3:42 PM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> > On Thu, Mar 15, 2012 at 04:14:03PM -0700, Daniel Farina wrote:
> >> I imagine the problem is a race condition whereby a pid might be
> >> reused by another process owned by another user (doesn't that also
> >> affect pg_cancel_backend?). ?Shall we just do everything using the
> >> MyCancelKey (which I think could just be called "SessionKey",
> >> "SessionSecret", or even just "Session") as to ensure we have no case
> >> of mistaken identity? Or does that end up being problematic?
> >
> > No, I think the hazard you identify here is orthogonal to the question of when
> > to authorize pg_terminate_backend(). ?As you note downthread, protocol-level
> > cancellations available in released versions already exhibit this hazard. ?I
> > wouldn't mind a clean fix for this, but it's an independent subject.
>
> Hmm. Well, here's a patch that implements exactly that, I think,
> similar to the one posted to this thread, but not using BackendIds,
> but rather the newly-introduced "SessionId". Would appreciate
> comments. Because an out-of-band signaling method has been integrated
> more complex behaviors -- such as closing the
> TERM-against-SECURITY-DEFINER-FUNCTION hazard -- can be addressed.
> For now I've only attempted to solve the problem of backend ambiguity,
> which basically necessitated out-of-line information transfer as per
> the usual means.

This patch still changes the policy for pg_terminate_backend(), and it does
not fix other SIGINT senders like processCancelRequest() and ProcSleep(). If
you're concerned about PID-reuse races, audit all backend signalling. Either
fix all such problems or propose a plan to get there eventually. Any further
discussion of this topic needs a new subject line; mixing its consideration
with proposals to change the policy behind pg_terminate_backend() reduces the
chances of the right people commenting on these distinct questions.

Currently, when pg_terminate_backend() follows a pg_cancel_backend() on which
the target has yet to act, the eventual outcome is a terminated process. With
this patch, the pg_terminate_backend() becomes a no-op with this warning:

> ! ereport(WARNING,
> ! (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> ! (errmsg("process is busy responding to administrative "
> ! "request")),
> ! (errhint("This is temporary, and may be retried."))));

That's less useful than the current behavior.

That being said, I can't get too excited about closing PID-reuse races. I've
yet to see another program do so. I've never seen a trouble report around
this race for any software. Every OS I have used assigns PIDs so as to
maximize the reuse interval, which seems like an important POLA measure given
typical admin formulae like "kill `ps | grep ...`". This defense can only
matter in fork-bomb conditions, at which point a stray signal is minor.

I do think it's worth keeping this idea in a back pocket for achieving those
"more complex behaviors," should we ever desire them.

> > Here I discussed a hazard specific to allowing pg_terminate_backend():
> > http://archives.postgresql.org/message-id/20110602045955.GC8246@tornado.gateway.2wire.net
> >
> > To summarize, user code can trap SIGINT cancellations, but it cannot trap
> > SIGTERM terminations. ?If a backend is executing a SECURITY DEFINER function
> > when another backend of the same role calls pg_terminate_backend() thereon,
> > the pg_terminate_backend() caller could achieve something he cannot achieve in
> > PostgreSQL 9.1. ?I vote that this is an acceptable loss.
>
> I'll throw out a patch that just lets this hazard exist and see what
> happens, although it is obsoleted/incompatible with the one already
> attached.

+1. Has anyone actually said that the PID-reuse race or the thing I mention
above should block such a patch? I poked back through the threads I could
remember and found nothing.

On Thu, Mar 15, 2012 at 9:39 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Fri, Mar 16, 2012 at 8:14 AM, Daniel Farina <daniel(at)heroku(dot)com> wrote:
>> Parallel to pg_cancel_backend, it'd be nice to allow the user to just
>> outright kill a backend that they own (politely, with a SIGTERM),
>> aborting any transactions in progress, including the idle transaction,
>> and closing the socket.
>
> +1

Here's a patch implementing the simple version, with no more guards
against signal racing than have been seen previously. The more
elaborate variants to close those races is being discussed in a
parallel thread, but I thought I'd get this simple version out there.

--
fdr

On Tue, 2012-03-20 at 01:38 -0700, Daniel Farina wrote:
> On Thu, Mar 15, 2012 at 9:39 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> > On Fri, Mar 16, 2012 at 8:14 AM, Daniel Farina <daniel(at)heroku(dot)com> wrote:
> >> Parallel to pg_cancel_backend, it'd be nice to allow the user to just
> >> outright kill a backend that they own (politely, with a SIGTERM),
> >> aborting any transactions in progress, including the idle transaction,
> >> and closing the socket.
> >
> > +1
>
> Here's a patch implementing the simple version, with no more guards
> against signal racing than have been seen previously. The more
> elaborate variants to close those races is being discussed in a
> parallel thread, but I thought I'd get this simple version out there.

Review:

After reading through the threads, it looks like there was no real
objection to this approach -- pid recycling is not something we're
concerned about.

I think you're missing a doc update though, in func.sgml:

"For the less restrictive <function>pg_cancel_backend</>, the role of an
active backend can be found from
the <structfield>usename</structfield> column of the
<structname>pg_stat_activity</structname> view."

Also, high-availability.sgml makes reference to pg_cancel_backend(), and
it might be worthwhile to add an "...and pg_terminate_backend()" there.

Other than that, it looks good to me.

Regards,
Jeff Davis

On Mon, Mar 26, 2012 at 12:14 AM, Jeff Davis <pgsql(at)j-davis(dot)com> wrote:
> On Tue, 2012-03-20 at 01:38 -0700, Daniel Farina wrote:
>> On Thu, Mar 15, 2012 at 9:39 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>> > On Fri, Mar 16, 2012 at 8:14 AM, Daniel Farina <daniel(at)heroku(dot)com> wrote:
>> >> Parallel to pg_cancel_backend, it'd be nice to allow the user to just
>> >> outright kill a backend that they own (politely, with a SIGTERM),
>> >> aborting any transactions in progress, including the idle transaction,
>> >> and closing the socket.
>> >
>> > +1
>>
>> Here's a patch implementing the simple version, with no more guards
>> against signal racing than have been seen previously.  The more
>> elaborate variants to close those races is being discussed in a
>> parallel thread, but I thought I'd get this simple version out there.
>
> Review:
>
> After reading through the threads, it looks like there was no real
> objection to this approach -- pid recycling is not something we're
> concerned about.
>
> I think you're missing a doc update though, in func.sgml:
>
> "For the less restrictive <function>pg_cancel_backend</>, the role of an
> active backend can be found from
> the <structfield>usename</structfield> column of the
> <structname>pg_stat_activity</structname> view."
>
> Also, high-availability.sgml makes reference to pg_cancel_backend(), and
> it might be worthwhile to add an "...and pg_terminate_backend()" there.
>
> Other than that, it looks good to me.

Good comments. Patch attached to address the doc issues. The only
iffy thing is that the paragraph "For the less restrictive..." I have
opted to remove in its entirely. I think the documents are already
pretty clear about the same-user rule, and I'm not sure if this is the
right place for a crash-course on attributes in pg_stat_activity (but
maybe it is).

"...and pg_terminate_backend" seems exactly right.

And I think now that the system post-patch doesn't have such a strange
contrast between the ability to send SIGINT vs. SIGTERM, such a
contrast may not be necessary.

I'm also not sure what the policy is about filling paragraphs in the
manual. I filled one, which increases the sgml churn a bit. git
(show|diff) --word-diff helps clean it up.

--
fdr

On Thu, Mar 29, 2012 at 3:04 AM, Daniel Farina <daniel(at)heroku(dot)com> wrote:
> On Mon, Mar 26, 2012 at 12:14 AM, Jeff Davis <pgsql(at)j-davis(dot)com> wrote:
>> On Tue, 2012-03-20 at 01:38 -0700, Daniel Farina wrote:
>>> On Thu, Mar 15, 2012 at 9:39 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>>> > On Fri, Mar 16, 2012 at 8:14 AM, Daniel Farina <daniel(at)heroku(dot)com> wrote:
>>> >> Parallel to pg_cancel_backend, it'd be nice to allow the user to just
>>> >> outright kill a backend that they own (politely, with a SIGTERM),
>>> >> aborting any transactions in progress, including the idle transaction,
>>> >> and closing the socket.
>>> >
>>> > +1
>>>
>>> Here's a patch implementing the simple version, with no more guards
>>> against signal racing than have been seen previously.  The more
>>> elaborate variants to close those races is being discussed in a
>>> parallel thread, but I thought I'd get this simple version out there.
>>
>> Review:
>>
>> After reading through the threads, it looks like there was no real
>> objection to this approach -- pid recycling is not something we're
>> concerned about.
>>
>> I think you're missing a doc update though, in func.sgml:
>>
>> "For the less restrictive <function>pg_cancel_backend</>, the role of an
>> active backend can be found from
>> the <structfield>usename</structfield> column of the
>> <structname>pg_stat_activity</structname> view."
>>
>> Also, high-availability.sgml makes reference to pg_cancel_backend(), and
>> it might be worthwhile to add an "...and pg_terminate_backend()" there.
>>
>> Other than that, it looks good to me.
>
> Good comments. Patch attached to address the doc issues.  The only
> iffy thing is that the paragraph "For the less restrictive..." I have
> opted to remove in its entirely.  I think the documents are already
> pretty clear about the same-user rule, and I'm not sure if this is the
> right place for a crash-course on attributes in pg_stat_activity (but
> maybe it is).
>
> "...and pg_terminate_backend" seems exactly right.
>
> And I think now that the system post-patch doesn't have such a strange
> contrast between the ability to send SIGINT vs. SIGTERM, such a
> contrast may not be necessary.
>
> I'm also not sure what the policy is about filling paragraphs in the
> manual.  I filled one, which increases the sgml churn a bit. git
> (show|diff) --word-diff helps clean it up.

I went ahead and committed this.

I kinda think we should back-patch this into 9.2. It doesn't involve
a catalog change, and would make the behavior consistent between the
two releases, instead of changing in 9.1 and then changing again in
9.2. Thoughts?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Jun 26, 2012 at 1:58 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Thu, Mar 29, 2012 at 3:04 AM, Daniel Farina <daniel(at)heroku(dot)com> wrote:
>> On Mon, Mar 26, 2012 at 12:14 AM, Jeff Davis <pgsql(at)j-davis(dot)com> wrote:
>>> On Tue, 2012-03-20 at 01:38 -0700, Daniel Farina wrote:
>>>> On Thu, Mar 15, 2012 at 9:39 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>>>> > On Fri, Mar 16, 2012 at 8:14 AM, Daniel Farina <daniel(at)heroku(dot)com> wrote:
>>>> >> Parallel to pg_cancel_backend, it'd be nice to allow the user to just
>>>> >> outright kill a backend that they own (politely, with a SIGTERM),
>>>> >> aborting any transactions in progress, including the idle transaction,
>>>> >> and closing the socket.
>>>> >
>>>> > +1
>>>>
>>>> Here's a patch implementing the simple version, with no more guards
>>>> against signal racing than have been seen previously. The more
>>>> elaborate variants to close those races is being discussed in a
>>>> parallel thread, but I thought I'd get this simple version out there.
>>>
>>> Review:
>>>
>>> After reading through the threads, it looks like there was no real
>>> objection to this approach -- pid recycling is not something we're
>>> concerned about.
>>>
>>> I think you're missing a doc update though, in func.sgml:
>>>
>>> "For the less restrictive <function>pg_cancel_backend</>, the role of an
>>> active backend can be found from
>>> the <structfield>usename</structfield> column of the
>>> <structname>pg_stat_activity</structname> view."
>>>
>>> Also, high-availability.sgml makes reference to pg_cancel_backend(), and
>>> it might be worthwhile to add an "...and pg_terminate_backend()" there.
>>>
>>> Other than that, it looks good to me.
>>
>> Good comments. Patch attached to address the doc issues. The only
>> iffy thing is that the paragraph "For the less restrictive..." I have
>> opted to remove in its entirely. I think the documents are already
>> pretty clear about the same-user rule, and I'm not sure if this is the
>> right place for a crash-course on attributes in pg_stat_activity (but
>> maybe it is).
>>
>> "...and pg_terminate_backend" seems exactly right.
>>
>> And I think now that the system post-patch doesn't have such a strange
>> contrast between the ability to send SIGINT vs. SIGTERM, such a
>> contrast may not be necessary.
>>
>> I'm also not sure what the policy is about filling paragraphs in the
>> manual. I filled one, which increases the sgml churn a bit. git
>> (show|diff) --word-diff helps clean it up.
>
> I went ahead and committed this.
>
> I kinda think we should back-patch this into 9.2. It doesn't involve
> a catalog change, and would make the behavior consistent between the
> two releases, instead of changing in 9.1 and then changing again in
> 9.2. Thoughts?

I think that would be good.

It is at the level of pain whereas I would backpatch from day one, but
I think it would be a welcome change to people who couldn't justify
gnashing their teeth and distributing a tweaked Postgres like I would.
It saves me some effort, too.

--
fdr

On Tue, Jun 26, 2012 at 10:58 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Thu, Mar 29, 2012 at 3:04 AM, Daniel Farina <daniel(at)heroku(dot)com> wrote:
>> On Mon, Mar 26, 2012 at 12:14 AM, Jeff Davis <pgsql(at)j-davis(dot)com> wrote:
>>> On Tue, 2012-03-20 at 01:38 -0700, Daniel Farina wrote:
>>>> On Thu, Mar 15, 2012 at 9:39 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>>>> > On Fri, Mar 16, 2012 at 8:14 AM, Daniel Farina <daniel(at)heroku(dot)com> wrote:
>>>> >> Parallel to pg_cancel_backend, it'd be nice to allow the user to just
>>>> >> outright kill a backend that they own (politely, with a SIGTERM),
>>>> >> aborting any transactions in progress, including the idle transaction,
>>>> >> and closing the socket.
>>>> >
>>>> > +1
>>>>
>>>> Here's a patch implementing the simple version, with no more guards
>>>> against signal racing than have been seen previously.  The more
>>>> elaborate variants to close those races is being discussed in a
>>>> parallel thread, but I thought I'd get this simple version out there.
>>>
>>> Review:
>>>
>>> After reading through the threads, it looks like there was no real
>>> objection to this approach -- pid recycling is not something we're
>>> concerned about.
>>>
>>> I think you're missing a doc update though, in func.sgml:
>>>
>>> "For the less restrictive <function>pg_cancel_backend</>, the role of an
>>> active backend can be found from
>>> the <structfield>usename</structfield> column of the
>>> <structname>pg_stat_activity</structname> view."
>>>
>>> Also, high-availability.sgml makes reference to pg_cancel_backend(), and
>>> it might be worthwhile to add an "...and pg_terminate_backend()" there.
>>>
>>> Other than that, it looks good to me.
>>
>> Good comments. Patch attached to address the doc issues.  The only
>> iffy thing is that the paragraph "For the less restrictive..." I have
>> opted to remove in its entirely.  I think the documents are already
>> pretty clear about the same-user rule, and I'm not sure if this is the
>> right place for a crash-course on attributes in pg_stat_activity (but
>> maybe it is).
>>
>> "...and pg_terminate_backend" seems exactly right.
>>
>> And I think now that the system post-patch doesn't have such a strange
>> contrast between the ability to send SIGINT vs. SIGTERM, such a
>> contrast may not be necessary.
>>
>> I'm also not sure what the policy is about filling paragraphs in the
>> manual.  I filled one, which increases the sgml churn a bit. git
>> (show|diff) --word-diff helps clean it up.
>
> I went ahead and committed this.
>
> I kinda think we should back-patch this into 9.2.  It doesn't involve
> a catalog change, and would make the behavior consistent between the
> two releases, instead of changing in 9.1 and then changing again in
> 9.2.  Thoughts?

+1.

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On Wed, Jun 27, 2012 at 8:13 AM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
>> I went ahead and committed this.
>>
>> I kinda think we should back-patch this into 9.2.  It doesn't involve
>> a catalog change, and would make the behavior consistent between the
>> two releases, instead of changing in 9.1 and then changing again in
>> 9.2.  Thoughts?
>
> +1.

Three votes with no objections is good enough for me, so, done.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company