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@heroku.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.fujii@gmail.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@heroku.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@sss.pgh.pa.us> wrote:
Daniel Farina <daniel@heroku.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@heroku.com> writes:
On Thu, Mar 15, 2012 at 10:33 PM, Tom Lane <tgl@sss.pgh.pa.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@sss.pgh.pa.us> wrote:
Daniel Farina <daniel@heroku.com> writes:
On Thu, Mar 15, 2012 at 10:33 PM, Tom Lane <tgl@sss.pgh.pa.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@heroku.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
Attachments:
Implement-race-free-sql-originated-backend-cancellation.patchtext/x-patch; charset=US-ASCII; name=Implement-race-free-sql-originated-backend-cancellation.patchDownload
From f466fe53e8e64cfa49bf56dbdf5920f9ea4e3562 Mon Sep 17 00:00:00 2001
From: Daniel Farina <daniel@heroku.com>
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 <daniel@heroku.com>
---
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))
+ HandleAdminActionInterrupt();
+
if (CheckProcSignal(PROCSIG_RECOVERY_CONFLICT_DATABASE))
RecoveryConflictInterrupt(PROCSIG_RECOVERY_CONFLICT_DATABASE);
*** a/src/backend/storage/lmgr/proc.c
--- b/src/backend/storage/lmgr/proc.c
***************
*** 356,361 **** InitProcess(void)
--- 356,364 ----
MyProc->backendId = InvalidBackendId;
MyProc->databaseId = InvalidOid;
MyProc->roleId = InvalidOid;
+ SpinLockInit(&MyProc->adminMutex);
+ MyProc->adminAction = ADMIN_ACTION_NONE;
+ MyProc->adminBackendId = InvalidBackendId;
MyPgXact->inCommit = false;
MyPgXact->vacuumFlags = 0;
/* NB -- autovac launcher intentionally does not set IS_AUTOVACUUM */
*** a/src/backend/tcop/postgres.c
--- b/src/backend/tcop/postgres.c
***************
*** 64,69 ****
--- 64,70 ----
#include "storage/proc.h"
#include "storage/procsignal.h"
#include "storage/sinval.h"
+ #include "storage/spin.h"
#include "tcop/fastpath.h"
#include "tcop/pquery.h"
#include "tcop/tcopprot.h"
***************
*** 2783,2788 **** RecoveryConflictInterrupt(ProcSignalReason reason)
--- 2784,2843 ----
}
/*
+ * HandleAdminActionInterrupt: handle backend administrative actions
+ *
+ * Backend admin actions are secure and unambiguous compared to sending a
+ * signal directly to a process id because in order for the intended function
+ * of the interrupt to be applied a correct BackendId must be applied.
+ */
+ void
+ HandleAdminActionInterrupt(void)
+ {
+ int save_errno = errno;
+
+ if (!proc_exit_inprogress)
+ {
+ volatile PGPROC *myVolatileProc = MyProc;
+ BEAdminAction actionToPerform;
+ BackendId submittedBackendId;
+
+ /* Copy out fields that must be coherent in MyProc with haste */
+ SpinLockAcquire(&myVolatileProc->adminMutex);
+ actionToPerform = myVolatileProc->adminAction;
+ submittedBackendId = myVolatileProc->adminBackendId;
+ SpinLockRelease(&myVolatileProc->adminMutex);
+
+ /*
+ * Abort if the MyBackendId doesn't match the submitted one, or is
+ * invalid.
+ */
+ if (submittedBackendId != MyBackendId ||
+ MyBackendId == InvalidBackendId)
+ goto finish;
+
+ /* Perform the administrative action */
+ Assert(submittedBackendId == MyBackendId);
+ switch (actionToPerform)
+ {
+ case ADMIN_ACTION_NONE:
+ break;
+
+ case ADMIN_ACTION_CANCEL:
+ InterruptPending = true;
+ QueryCancelPending = true;
+ break;
+
+ case ADMIN_ACTION_TERMINATE:
+ die(SIGTERM);
+ break;
+ }
+ }
+
+ finish:
+ errno = save_errno;
+ }
+
+ /*
* ProcessInterrupts: out-of-line portion of CHECK_FOR_INTERRUPTS() macro
*
* If an interrupt condition is pending, and it's safe to service it,
*** a/src/backend/utils/adt/misc.c
--- b/src/backend/utils/adt/misc.c
***************
*** 32,37 ****
--- 32,38 ----
#include "storage/pmsignal.h"
#include "storage/proc.h"
#include "storage/procarray.h"
+ #include "storage/spin.h"
#include "utils/lsyscache.h"
#include "tcop/tcopprot.h"
#include "utils/builtins.h"
***************
*** 88,147 **** current_query(PG_FUNCTION_ARGS)
static int
pg_signal_backend(int pid, int sig)
{
! PGPROC *proc;
! if (!superuser())
{
! /*
! * Since the user is not superuser, check for matching roles. Trust
! * that BackendPidGetProc will return NULL if the pid isn't valid,
! * even though the check for whether it's a backend process is below.
! * The IsBackendPid check can't be relied on as definitive even if it
! * was first. The process might end between successive checks
! * regardless of their order. There's no way to acquire a lock on an
! * arbitrary process to prevent that. But since so far all the callers
! * of this mechanism involve some request for ending the process
! * anyway, that it might end on its own first is not a problem.
! */
! proc = BackendPidGetProc(pid);
- if (proc == NULL || proc->roleId != GetUserId())
- return SIGNAL_BACKEND_NOPERMISSION;
- }
-
- if (!IsBackendPid(pid))
- {
/*
! * This is just a warning so a loop-through-resultset will not abort
! * if one backend terminated on it's own during the run
*/
! ereport(WARNING,
! (errmsg("PID %d is not a PostgreSQL server process", pid)));
! return SIGNAL_BACKEND_ERROR;
}
/*
! * Can the process we just validated above end, followed by the pid being
! * recycled for a new process, before reaching here? Then we'd be trying
! * to kill the wrong thing. Seems near impossible when sequential pid
! * assignment and wraparound is used. Perhaps it could happen on a system
! * where pid re-use is randomized. That race condition possibility seems
! * too unlikely to worry about.
*/
! /* If we have setsid(), signal the backend's whole process group */
! #ifdef HAVE_SETSID
! if (kill(-pid, sig))
! #else
! if (kill(pid, sig))
! #endif
{
! /* Again, just a warning to allow loops */
! ereport(WARNING,
! (errmsg("could not send signal to process %d: %m", pid)));
! return SIGNAL_BACKEND_ERROR;
}
- return SIGNAL_BACKEND_SUCCESS;
}
/*
--- 89,167 ----
static int
pg_signal_backend(int pid, int sig)
{
! volatile PGPROC *proc;
! /* Guard against unexpected signals */
! switch (sig)
{
! default:
! /* Should never happen. */
! ereport(FATAL,
! (errcode(ERRCODE_INTERNAL_ERROR),
! errmsg("unexpected signal passed to %s",
! PG_FUNCNAME_MACRO)));
/*
! * The following are the only expected signals this function can be
! * called with.
*/
! case SIGINT:
! case SIGTERM:
! /* fall-through */
! ;
}
/*
! * Set another backend's BEAdminAction if the current backend is
! * controlledb by a superuser or a user of the same role of the other
! * backend.
! *
! * Trust that BackendPidGetProc will return NULL if the pid isn't
! * valid, even though the check for whether it's a backend process is
! * below. The IsBackendPid check can't be relied on as definitive even
! * if it was first. The process might end between successive checks
! * regardless of their order. There's no way to acquire a lock on an
! * arbitrary process to prevent that. But since so far all the callers
! * of this mechanism involve some request for ending the process
! * anyway, that it might end on its own first is not a problem.
*/
+ proc = BackendPidGetProc(pid);
! if (proc == NULL)
! return SIGNAL_BACKEND_NOPERMISSION;
! else
{
! const BackendId backendId = proc->backendId;
! const Oid roleId = proc->roleId;
!
! if (roleId != GetUserId())
! return SIGNAL_BACKEND_NOPERMISSION;
!
! Assert(roleId == GetUserId() || superuser());
!
! SpinLockAcquire(&proc->adminMutex);
! proc->adminBackendId = backendId;
!
! switch (sig)
! {
! case SIGINT:
! proc->adminAction = ADMIN_ACTION_CANCEL;
! break;
! case SIGTERM:
! proc->adminAction = ADMIN_ACTION_TERMINATE;
! break;
! default:
! break;
! }
!
! SpinLockRelease(&proc->adminMutex);
!
! if (SendProcSignal(proc->pid, PROCSIG_ADMIN_ACTION_INTERRUPT,
! backendId) < 0)
! return SIGNAL_BACKEND_ERROR;
! else
! return SIGNAL_BACKEND_SUCCESS;
}
}
/*
*** a/src/include/miscadmin.h
--- b/src/include/miscadmin.h
***************
*** 247,252 **** extern bool VacuumCostActive;
--- 247,253 ----
/* in tcop/postgres.c */
extern void check_stack_depth(void);
+ extern void HandleAdminActionInterrupt(void);
/* in tcop/utility.c */
extern void PreventCommandIfReadOnly(const char *cmdname);
*** a/src/include/storage/proc.h
--- b/src/include/storage/proc.h
***************
*** 19,24 ****
--- 19,25 ----
#include "storage/latch.h"
#include "storage/lock.h"
#include "storage/pg_sema.h"
+ #include "storage/s_lock.h"
/*
* Each backend advertises up to PGPROC_MAX_CACHED_SUBXIDS TransactionIds
***************
*** 55,60 **** struct XidCache
--- 56,68 ----
*/
#define FP_LOCK_SLOTS_PER_BACKEND 16
+ /* The available administrative actions that can be performed on a backend */
+ typedef enum BEAdminAction {
+ ADMIN_ACTION_NONE = 0,
+ ADMIN_ACTION_CANCEL,
+ ADMIN_ACTION_TERMINATE
+ } BEAdminAction;
+
/*
* Each backend has a PGPROC struct in shared memory. There is also a list of
* currently-unused PGPROC structs that will be reallocated to new backends.
***************
*** 93,98 **** struct PGPROC
--- 101,115 ----
Oid roleId; /* OID of role using this backend */
/*
+ * Backend administration fields
+ *
+ * adminActionMutex protects reading/writing out the admin fields
+ */
+ slock_t adminMutex;
+ BEAdminAction adminAction;
+ BackendId adminBackendId;
+
+ /*
* While in hot standby mode, shows that a conflict signal has been sent
* for the current transaction. Set/cleared while holding ProcArrayLock,
* though not required. Accessed without lock, if needed.
*** a/src/include/storage/procsignal.h
--- b/src/include/storage/procsignal.h
***************
*** 31,36 **** typedef enum
--- 31,37 ----
{
PROCSIG_CATCHUP_INTERRUPT, /* sinval catchup interrupt */
PROCSIG_NOTIFY_INTERRUPT, /* listen/notify interrupt */
+ PROCSIG_ADMIN_ACTION_INTERRUPT, /* trigger backend administrative action */
/* Recovery conflict reasons */
PROCSIG_RECOVERY_CONFLICT_DATABASE,
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@leadboat.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.netTo 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
Attachments:
Implement-race-free-sql-originated-backend-cancellation-v2.patch.gzapplication/x-gzip; name=Implement-race-free-sql-originated-backend-cancellation-v2.patch.gzDownload
���cO Implement-race-free-sql-originated-backend-cancellation-v2.patch �\�w"�����{������.�x���v �M��=����~�V�6���U%��0�{s�������JR��*U�.�]�[v�q^?�O���}���y�[��V��N�&�V���'�c�`�3V�_��Y�7�w@���Z�����Y�����>���M|�}��
�%�D�5N�'+���&k�/[���SV�5���h�7> /�>������z���.�BX^��3��S�1��Bn��5y��]d��p��B�{��.v������l��s!+Lx�H�b_��C!<dO����
4��0�gW�r.&s&d1���g�+�gvm�O�{���Y��C��)�oe�h[�5�$?*&d����eY��������<?�[-����������7���1���l��5���]������K��
��z�4��`1��pfIz`��IOH6���!c���N��1�R"���m�Yp\�+��E�P23���
���&������`���� s�
c�U�����&���96��:
v���Bs��;x�E�h��+N9�}�=������cs)kT�~�4���MW��h-G��6bR�O� |xj�H���s����'����JZC��D�&�4;l�����<�$$p� ��&W|�����<�E�N2�&��(���Tg��v���@�����ZU`�$�la��_�����a��P��z�����4�s������������H����4�Z���$����6�d���$Ya���fr)�S�V�Q�{6�y�;�����b�A�����XX�� �`O��#�
�z�J?��i�M�*��]��/��A��f/|%��^��p�^��un-�p�3�84�a��beR�����?�V�����j�Z�C��d ��\���=�������� ;��Y����!e-,��.��m�$��/C?�f�&����%[tW����w�Y@�k� 0��k����-|�@���f�)0���P8�f�a�r� �������j�O�C�D6'*�
2x4�ce����7;�M���7[,Yr�����l*� H)�K�5ON@m�@����a������9\=������f������5d��F�U?������{��A����T�J�6B�N��~��*��2|L����U1a��#@�-���Q��y��t���M��~!��Z��]��/�����|]��\���|5XV�J=�V������k�-����yh��B�p��u-���W�8�����?Z��&|5P����pb��Y=��O�P{c�-�wB*���H��Y�#��@�q��M2;�%��z;���@��`YnK�rc�{����y�J��de�2��o�n�6+�g��D
��a���3l?mh� �VG����T^-���`���0�m��2;NY�q$p�2���.����p�����<7O+��
e� TxiK�M������Hr ���
&N�i�f��w�<"��u�T��8F?�g������A���|�(���/=\�
��=���'����VO��<�'P�/e�U;���uW� B�H��H7�x��p�"!G'0w�j������}���0jw�������C7�tg�WKJ�A���O>��_k��4<<��_�h�6,��[�� D����<=V���G$��<����`��}?j�?�4�����Rg�'/x6"RB6�@���w��@y��~��8���h?�6{��z%�}��]3���{�m'��!~����~������e�y|���u�����}�t���|������0������;�6�+���VZ�Qkp��|����P�:��0�T}��|���.O���:��M�_�$<�t�B*Vm�a(�`G�X�Nfb�{au�Zy��)Hq���}�,�����\J}fEx0C��v��x����S �l
������Ui�_d� ��X�<uH���[x�1���-
6�8�I&h
�h�21�)Bi}���<�m V����J�O[�]�2L0��m�Ov�����c�B�a��j,��)dR�'�D BM��*�)���������������p�� \H��*�@��$p��\���W�!��������q�f6�TR�`N���{�i�ci������ktx��z�=%1d~����'�����gkE�Y8������.N;7z���%5>��^�t��Y��Qd���6�~����C���7����]~t���CV+���
`����3�6i~�s�s����N>�`B������ �"�E��p]�|W���J�f������s���� t�t���9f{�G�P���^��i�rV��N���t���~6�LA{13�����Tn��E��Pi����?��F�p����sKbZ�>� >#3}s�e���2�X�a�%S0'��H��1�������(Z7����l���
M'4��j`�]@���IIIAEK�E�U����5�\a��&����hPIjm�+��M���qav�1n[,�-XK�S�$�v��B� >v *e��o���J������A����y4�{��f|G���Q!$�6"��}����~�
��d,
7���
����G���|c�P��u#�vlW���4 ���bJ:���s���
��RC���'�Fp����K���f���X1Lkr��������(c�=�R�s�Cnf^�o"k4�9�+)�l�a��\>u(k.��'���� ��Y������{��q�Y��}��of=}����>�N1Mg�a���;8B��fX�|�W�i�hDz���������~��������A��R�Ma��dN�����*���Z/����t���m�\�� ���&xjiJ��2�<�K�X��~�����~�t�M���<i����_�����t�f!��W����C{/���o~�T
�?����:h�]b����G�[�o`h�'y��_��U�*��s������8�{�'�� @�kM�h)��)��%�>�1�&�u�F�[��r��W�Za%���
����.� � 6�3�uW����X��w�L��%���0�Wq��D]..*�V��V�r�2@r6���[�.����X���@sC�J��B�F�5��z��aVF2�E�q�:��Y��s���PV6[��a���� e�����O�"�hKE+R�G��s��wbN^��Tb?����S��8�g��B�*�h$� ����L�R�����+�3��!�>KOFw�J�U�*C@�� .t�*�g�U���x{u���E��=�����J�3�����H�T
�� 5�-v��mL �u�D�e� � E���X�K��8���x��v$>%�T?,6��+�����<���u�w��5�6��{@l%����4�e��R���.0��=���0k�+4�=e,YZ��ta^�S@8����^��O�=���2m?���� % ��X8"h5U�� Y���p�^�|:�i��N}{����!�0� ���c�f�,f#�a5�R���k'�,f(��A��F���!^�=����/�u&bO!�=JL�I"���Q��$�AV���#6���C��j�$�<�P�q�:N2PF3`0b'�DC�:��L��d������XE��Khyd(�j�d�"Mjb��X��)���\t��h�Jo
�C�"��53�b6�R�p�y�H�&��B�gV q�R1�T�FNe&0�
�Tt {t ��=C�
��Z��W�JR���B}�!� �;�D��5�B�.#��a������&Y�S�l>^���]�� B�P������,
X�w�A��s�B�
*����@�D��*��e�� ��g�����
Z�vh�v�$@U�MZrh
elRi�Dy����o��+%}:<Q
��rUUa����!�!s�A�oi�����;�H;
�9�Gk1�!BTX�k���{����P(�
t����n�;;9�'�b�RJ��d�+��[&�l����i{L�v;-(�&�����`��f�+���,��b�@>O��i6I���t1�?��! ��(��C}�`�w���UB4�Eh]X�rw���8w�1�
w��ce�)IT�*��`q��%�2S��2���*�����O�JI�����&uf�%C�jjT�G[x��7X����+�H��[ku^j��{I\}�G���R*�Z��
�);R]j��aP �C�� ��\�O�B@��s'�H�B^{G1E�����;t����q<��R���*T��P>����CO�(��z���zQ��
��|��SP�Z8���m}g_���o���f!���W��"��A<��u�tgx��*Zk�k+(q������S�9�$�t�6���[+���m���d:@hT�Fd����!t�p��Y��D�"K��9�
�{\���8��D`�O;��]�h�|w������S���w���-��, hd0x��c�HAp���S����%k2��/k�QhC��������>w�Z4�;e#�������G4�J�g��&�l7k���j����m?��v��8>R��;,��"(}^� 9�x�J�n
w�h���Y�)������o/�k�o�7�d������I�X��eN#/�T����+-*~��ma�Q����Y)����Ft���`q�5�c��Ef|c�_�P)d���
x�.���d�$��K,Yo���K��w�j) ��N��~��DE������D��@��������jB_�
@
V��047����-
�~jroB�}����j ��4/� ����
aIS������"pn����������\T�k<�j+�����B!]������G<
1���t�� ��P�}�={C ��O�U�=���1���$����\��kSs]������T��T�^�x�@W�u������n!�MX�K�R���Iol�&�
T�T ��������R-@'XX|�xD2{t�EpPGr����47�^s8��`�C�b`*�E��px_Ze�*�L�um�I�u��W��S-����2R�H��5�=���Y]���e[x�� �T�j.hS��
S��!I��sK\�c��~�l+k�I3�m�2��o���b�/��n���so�vG�a{�Ds��8�K;�H�4l�M� {���>�}�W X#��
{�w>z;u���c$X�}�AiO :;��u�D�)<�����QQ���i��r���A����o
�r��c�L�@@���-��%;����e����J� -f��6�
�����[S
���
���1C+��������a�o�Nc3 /tU���q:���q�iSc�X[�������N��
��yO�K��|�?�}�ghX\��/�dI�������gI
P���������r��Y�y�T������H���;����+U�K��3�3z�~��f�?M�Nr��g����\�b�U����W�6&�����|R�0������-��R�M��k{ ����f�r���!����J3UY�_O��V9x���v
� [���(q��ZU5J�r�]LP����������*5��� B�c��7?�n,�������I����
`����`����4U2��N�h
wO���������p0����8U�U5���^-�����/Wi �� ��Q�y�]S�S�Pk-�FE[�_\���6���_UV���zG� �rP��w�y��o��$5�A+��j�]���J������6a
*]��������Yr��&V\����A<�.�������.�����(�J��J�q�5���0W�>�n1Qe�W��lfD�D���N����p�\��s���g���6�E����Y��GK����)��Z�(��_�{�F+�^��">���BL����{����^t�UrY�2�={���.^[�7N��������f^� ���"�]�+#a@7����S�������AV����>J�5`��pLv���Mm���<��L��o��9���������Q"m�?;�WH�L��9��?8��mT+�����p�zq��wM�ek:�*�t� ���R�U!K��3��j�IIt�2"�G���Yk�O:�d�$�*�������E\&�������Q�����n��[����`R56��q,����
6%���9����� �i4@�1��8Q�7�I��.�7Z�E�\�c�Z�
�U�����/���#q]f��d��2��m_���^����{���[_��e
Yj��� B�����N@UX/�M
Y{��r�r��w��L On Fri, Mar 16, 2012 at 4:42 PM, Daniel Farina <daniel@heroku.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
Attachments:
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@leadboat.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.netTo 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.fujii@gmail.com> wrote:
On Fri, Mar 16, 2012 at 8:14 AM, Daniel Farina <daniel@heroku.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
Attachments:
Extend-same-role-backend-management-to-pg_terminate_backend.patchtext/x-patch; charset=US-ASCII; name=Extend-same-role-backend-management-to-pg_terminate_backend.patchDownload
From 73c794a0cce148c2848adfb06be9aac985ac41d8 Mon Sep 17 00:00:00 2001
From: Daniel Farina <daniel@heroku.com>
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 <daniel@heroku.com>
---
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);
<literal><function>pg_terminate_backend(<parameter>pid</parameter> <type>int</>)</function></literal>
</entry>
<entry><type>boolean</type></entry>
! <entry>Terminate a backend</entry>
</row>
</tbody>
</tgroup>
--- 14403,14413 ----
<literal><function>pg_terminate_backend(<parameter>pid</parameter> <type>int</>)</function></literal>
</entry>
<entry><type>boolean</type></entry>
! <entry>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.
! </entry>
</row>
</tbody>
</tgroup>
*** 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);
}
/*
On Tue, 2012-03-20 at 01:38 -0700, Daniel Farina wrote:
On Thu, Mar 15, 2012 at 9:39 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Fri, Mar 16, 2012 at 8:14 AM, Daniel Farina <daniel@heroku.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@j-davis.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.fujii@gmail.com> wrote:
On Fri, Mar 16, 2012 at 8:14 AM, Daniel Farina <daniel@heroku.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
Attachments:
Extend-same-role-backend-management-to-pg_terminate_backend-v2.patchapplication/octet-stream; name=Extend-same-role-backend-management-to-pg_terminate_backend-v2.patchDownload
From 43b6af42a90d96919a22c035d897cba2fbe46561 Mon Sep 17 00:00:00 2001
From: Daniel Farina <daniel@heroku.com>
Date: Wed, 28 Mar 2012 23:54:50 -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 <daniel@heroku.com>
---
doc/src/sgml/func.sgml | 10 +++++-----
doc/src/sgml/high-availability.sgml | 16 +++++++++-------
src/backend/utils/adt/misc.c | 12 +++++++-----
3 files changed, 21 insertions(+), 17 deletions(-)
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 34fea16..1a7b8ec 100644
*** a/doc/src/sgml/func.sgml
--- b/doc/src/sgml/func.sgml
***************
*** 14403,14409 **** SELECT set_config('log_statement_stats', 'off', false);
<literal><function>pg_terminate_backend(<parameter>pid</parameter> <type>int</>)</function></literal>
</entry>
<entry><type>boolean</type></entry>
! <entry>Terminate a backend</entry>
</row>
</tbody>
</tgroup>
--- 14403,14413 ----
<literal><function>pg_terminate_backend(<parameter>pid</parameter> <type>int</>)</function></literal>
</entry>
<entry><type>boolean</type></entry>
! <entry>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.
! </entry>
</row>
</tbody>
</tgroup>
***************
*** 14424,14433 **** SELECT set_config('log_statement_stats', 'off', false);
<command>postgres</command> processes on the server (using
<application>ps</> on Unix or the <application>Task
Manager</> on <productname>Windows</>).
- 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.
</para>
<para>
--- 14428,14433 ----
*** a/doc/src/sgml/high-availability.sgml
--- b/doc/src/sgml/high-availability.sgml
***************
*** 1969,1981 **** LOG: database system is ready to accept read only connections
</para>
<para>
! <function>pg_cancel_backend()</> will work on user backends, but not the
! Startup process, which performs recovery. <structname>pg_stat_activity</structname> does not
! show an entry for the Startup process, nor do recovering transactions
! show as active. As a result, <structname>pg_prepared_xacts</structname> is always empty during
! recovery. If you wish to resolve in-doubt prepared transactions,
! view <literal>pg_prepared_xacts</> on the primary and issue commands to
! resolve transactions there.
</para>
<para>
--- 1969,1983 ----
</para>
<para>
! <function>pg_cancel_backend()</>
! and <function>pg_terminate_backend()</> will work on user backends,
! but not the Startup process, which performs
! recovery. <structname>pg_stat_activity</structname> does not show an
! entry for the Startup process, nor do recovering transactions show
! as active. As a result, <structname>pg_prepared_xacts</structname>
! is always empty during recovery. If you wish to resolve in-doubt
! prepared transactions, view <literal>pg_prepared_xacts</> on the
! primary and issue commands to resolve transactions there.
</para>
<para>
*** 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);
}
/*
On Thu, Mar 29, 2012 at 3:04 AM, Daniel Farina <daniel@heroku.com> wrote:
On Mon, Mar 26, 2012 at 12:14 AM, Jeff Davis <pgsql@j-davis.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.fujii@gmail.com> wrote:
On Fri, Mar 16, 2012 at 8:14 AM, Daniel Farina <daniel@heroku.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@gmail.com> wrote:
On Thu, Mar 29, 2012 at 3:04 AM, Daniel Farina <daniel@heroku.com> wrote:
On Mon, Mar 26, 2012 at 12:14 AM, Jeff Davis <pgsql@j-davis.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.fujii@gmail.com> wrote:
On Fri, Mar 16, 2012 at 8:14 AM, Daniel Farina <daniel@heroku.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@gmail.com> wrote:
On Thu, Mar 29, 2012 at 3:04 AM, Daniel Farina <daniel@heroku.com> wrote:
On Mon, Mar 26, 2012 at 12:14 AM, Jeff Davis <pgsql@j-davis.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.fujii@gmail.com> wrote:
On Fri, Mar 16, 2012 at 8:14 AM, Daniel Farina <daniel@heroku.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@hagander.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