Re: [HACKERS] pg_terminate_backend for same-role
On Wed, Jun 27, 2012 at 8:13 AM, Magnus Hagander 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_terminate_backend for same-role
On Tue, Jun 26, 2012 at 10:58 PM, Robert Haas wrote: > On Thu, Mar 29, 2012 at 3:04 AM, Daniel Farina wrote: >> On Mon, Mar 26, 2012 at 12:14 AM, Jeff Davis wrote: >>> On Tue, 2012-03-20 at 01:38 -0700, Daniel Farina wrote: On Thu, Mar 15, 2012 at 9:39 PM, Fujii Masao wrote: > On Fri, Mar 16, 2012 at 8:14 AM, 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 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 pg_cancel_backend, the role of an >>> active backend can be found from >>> the usename column of the >>> pg_stat_activity 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/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_terminate_backend for same-role
On Tue, Jun 26, 2012 at 1:58 PM, Robert Haas wrote: > On Thu, Mar 29, 2012 at 3:04 AM, Daniel Farina wrote: >> On Mon, Mar 26, 2012 at 12:14 AM, Jeff Davis wrote: >>> On Tue, 2012-03-20 at 01:38 -0700, Daniel Farina wrote: On Thu, Mar 15, 2012 at 9:39 PM, Fujii Masao wrote: > On Fri, Mar 16, 2012 at 8:14 AM, 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 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 pg_cancel_backend, the role of an >>> active backend can be found from >>> the usename column of the >>> pg_stat_activity 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_terminate_backend for same-role
On Thu, Mar 29, 2012 at 3:04 AM, Daniel Farina wrote: > On Mon, Mar 26, 2012 at 12:14 AM, Jeff Davis wrote: >> On Tue, 2012-03-20 at 01:38 -0700, Daniel Farina wrote: >>> On Thu, Mar 15, 2012 at 9:39 PM, Fujii Masao wrote: >>> > On Fri, Mar 16, 2012 at 8:14 AM, 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 >>> >>> 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 pg_cancel_backend, the role of an >> active backend can be found from >> the usename column of the >> pg_stat_activity 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_terminate_backend for same-role
On Mon, Mar 26, 2012 at 12:14 AM, Jeff Davis wrote: > On Tue, 2012-03-20 at 01:38 -0700, Daniel Farina wrote: >> On Thu, Mar 15, 2012 at 9:39 PM, Fujii Masao wrote: >> > On Fri, Mar 16, 2012 at 8:14 AM, 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 >> >> 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 pg_cancel_backend, the role of an > active backend can be found from > the usename column of the > pg_stat_activity 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 Extend-same-role-backend-management-to-pg_terminate_backend-v2.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_terminate_backend for same-role
On Tue, 2012-03-20 at 01:38 -0700, Daniel Farina wrote: > On Thu, Mar 15, 2012 at 9:39 PM, Fujii Masao wrote: > > On Fri, Mar 16, 2012 at 8:14 AM, 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 > > 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 pg_cancel_backend, the role of an active backend can be found from the usename column of the pg_stat_activity 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_terminate_backend for same-role
On Thu, Mar 15, 2012 at 9:39 PM, Fujii Masao wrote:
> On Fri, Mar 16, 2012 at 8:14 AM, 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
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
From 73c794a0cce148c2848adfb06be9aac985ac41d8 Mon Sep 17 00:00:00 2001
From: Daniel Farina
Date: Sun, 18 Mar 2012 20:08:37 -0700
Subject: [PATCH] Extend same-role backend management to pg_terminate_backend
This makes it more similar to pg_cancel_backend, except it gives users
the ability to close runaway connections entirely.
Signed-off-by: Daniel Farina
---
doc/src/sgml/func.sgml |6 +-
src/backend/utils/adt/misc.c | 12 +++-
2 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 34fea16..7cece3a 100644
*** a/doc/src/sgml/func.sgml
--- b/doc/src/sgml/func.sgml
***
*** 14403,14409 SELECT set_config('log_statement_stats', 'off', false);
pg_terminate_backend(pid int)
boolean
!Terminate a backend
--- 14403,14413
pg_terminate_backend(pid int)
boolean
!Terminate a backend. You can execute this against
! another backend that has exactly the same role as the user
! calling the function. In all other cases, you must be a
! superuser.
!
*** a/src/backend/utils/adt/misc.c
--- b/src/backend/utils/adt/misc.c
***
*** 162,179 pg_cancel_backend(PG_FUNCTION_ARGS)
}
/*
! * Signal to terminate a backend process. Only allowed by superuser.
*/
Datum
pg_terminate_backend(PG_FUNCTION_ARGS)
{
! if (!superuser())
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
! errmsg("must be superuser to terminate other server processes"),
! errhint("You can cancel your own processes with pg_cancel_backend().")));
! PG_RETURN_BOOL(pg_signal_backend(PG_GETARG_INT32(0), SIGTERM) == SIGNAL_BACKEND_SUCCESS);
}
/*
--- 162,181
}
/*
! * Signal to terminate a backend process. This is allowed if you are superuser
! * or have the same role as the process being terminated.
*/
Datum
pg_terminate_backend(PG_FUNCTION_ARGS)
{
! int r = pg_signal_backend(PG_GETARG_INT32(0), SIGTERM);
!
! if (r == SIGNAL_BACKEND_NOPERMISSION)
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
! (errmsg("must be superuser or have the same role to terminate backends running in other server processes";
! PG_RETURN_BOOL(r == SIGNAL_BACKEND_SUCCESS);
}
/*
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_terminate_backend for same-role
On Fri, Mar 16, 2012 at 04:42:07PM -0700, Daniel Farina wrote:
> On Fri, Mar 16, 2012 at 3:42 PM, Noah Misch 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.gc8...@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.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_terminate_backend for same-role
On Fri, Mar 16, 2012 at 4:42 PM, Daniel Farina 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 Implement-race-free-sql-originated-backend-cancellation-v3.patch.gz Description: GNU Zip compressed data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_terminate_backend for same-role
On Fri, Mar 16, 2012 at 3:42 PM, Noah Misch 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.gc8...@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 Implement-race-free-sql-originated-backend-cancellation-v2.patch.gz Description: GNU Zip compressed data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_terminate_backend for same-role
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.gc8...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_terminate_backend for same-role
On Thu, Mar 15, 2012 at 11:01 PM, Daniel Farina 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 From f466fe53e8e64cfa49bf56dbdf5920f9ea4e3562 Mon Sep 17 00:00:00 2001 From: Daniel Farina Date: Thu, 15 Mar 2012 20:46:38 -0700 Subject: [PATCH] Implement race-free sql-originated backend cancellation/termination If promising, this patch requires a few documentation updates in addendum. Since 0495aaad8b337642830a4d4e82f8b8c02b27b1be, pg_cancel_backend can be run on backends that have the same role as the backend pg_cancel_backend is being invoked from. Since that time, a documented caveat exists stating that there was a race whereby a process death-and-recycle could result in an otherwise unrelated backend receiving SIGINT. Now it is desirable for pg_terminate_backend -- which also has the effect of having the backend exit, and closing the socket -- to also be usable by non-superusers. Presuming SIGINT was acceptable to race, it's not clear that it's acceptable for SIGTERM to race in the same way, so this patch seeks to try to do something about that. This patch attempts to close this race condition by targeting query cancellation/termination against the per-backend-startup unique BackendId to unambiguously identify the session rather than a PID. This makes the SQL function act more like how cancellation keys work already (perhaps these paths can be converged somehow). Signed-off-by: Daniel Farina --- src/backend/access/transam/twophase.c |4 + src/backend/storage/ipc/procsignal.c |3 + src/backend/storage/lmgr/proc.c |3 + src/backend/tcop/postgres.c | 55 + src/backend/utils/adt/misc.c | 104 +++- src/include/miscadmin.h |1 + src/include/storage/proc.h| 17 + src/include/storage/procsignal.h |1 + 8 files changed, 146 insertions(+), 42 deletions(-) *** a/src/backend/access/transam/twophase.c --- b/src/backend/access/transam/twophase.c *** *** 62,67 --- 62,68 #include "storage/procarray.h" #include "storage/sinvaladt.h" #include "storage/smgr.h" + #include "storage/spin.h" #include "utils/builtins.h" #include "utils/memutils.h" #include "utils/timestamp.h" *** *** 326,331 MarkAsPreparing(TransactionId xid, const char *gid, --- 327,335 proc->backendId = InvalidBackendId; proc->databaseId = databaseid; proc->roleId = owner; + SpinLockInit(&MyProc->adminMutex); + proc->adminAction = ADMIN_ACTION_NONE; + proc->adminBackendId = InvalidBackendId; proc->lwWaiting = false; proc->lwWaitMode = 0; proc->lwWaitLink = NULL; *** a/src/backend/storage/ipc/procsignal.c --- b/src/backend/storage/ipc/procsignal.c *** *** 258,263 procsignal_sigusr1_handler(SIGNAL_ARGS) --- 258,266 if (CheckProcSignal(PROCSIG_NOTIFY_INTERRUPT)) HandleNotifyInterrupt(); + if (CheckProcSignal(PROCSIG_ADMIN_ACTION_INTERRUPT)) + HandleAdminActionInter
Re: [HACKERS] pg_terminate_backend for same-role
On Thu, Mar 15, 2012 at 10:45 PM, Tom Lane wrote: > Daniel Farina writes: >> On Thu, Mar 15, 2012 at 10:33 PM, Tom Lane 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_terminate_backend for same-role
Daniel Farina writes: > On Thu, Mar 15, 2012 at 10:33 PM, Tom Lane 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_terminate_backend for same-role
On Thu, Mar 15, 2012 at 10:33 PM, Tom Lane wrote: > Daniel Farina 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_terminate_backend for same-role
Daniel Farina 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_terminate_backend for same-role
On Thu, Mar 15, 2012 at 9:39 PM, Fujii Masao 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_terminate_backend for same-role
On Fri, Mar 16, 2012 at 8:14 AM, 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?). 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
