Re: Cross-backend signals and administration (Was: Re: pg_terminate_backend for same-role)

This thread evolved out of an attempt to implement
pg_terminate_backend for non-superusers. I thought -- probably
erroneously -- that the major objection to that was the known
possibility of a PID-cycling race condition, whereby a signal could be
misdirected, in the case of terminate_backend, SIGTERM. So now this
fork of the thread is about fixing these unlikely races, and then
passing administration requests (such as "query cancel" or "die" ) as
out-of-band information via SIGUSR1, just like how LISTEN/NOTIFY and
conflict signals are passed.

To prevent ambiguity, I am using a new special number -- a 'SessionId'
-- that is guaranteed unique to all backends ever created during the
uptime of a database. This number is currently implemented in a way
that is guessable (so it cannot be accepted from external sources),
but I actually think it may be even more useful for a number of other
uses if given a non-guessable form (like cancellation keys). In this
respect it would fulfill pretty much the same purposes as the notion
of a "session" seen on the web.

Noah offered me these comments:
> 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.

Is the postmaster signaling its children intrinsically vulnerable to
PID racing? Because it controls when it can call wait() or waitpid()
on child processes, it can unambiguously know that PIDs have not been
cycled for use. For this reason, a credible and entirely alternate
design might be to bounce IPC requests through the postmaster, but
since postmaster is so critical I had decided not to introduce nor
change mechanics there.

The Postmaster I think keeps a private copy of cancellation keys that
are not in shared memory, if I read it properly (not 100% sure), and
uses that for cancellation requests. This has a useful property of
allowing cancellations even in event that shared memory goes insane
(and since postmaster is typically left as last sane process of the
group I thought it wise to not have it reuse a shared-memory based
approach).

I cannot comment on ProcSleep at this time.

> 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.

Yes. It could be fixed with dynamic allocation (holding more
administration requests), but for just getting a flavor of what a
solution might be like. I wanted to avoid additional dynamic
allocation (which would necessitate a similar condition in the form of
much-less likely OOM), but at some point I think this error condition
is inevitable in some form. I see it as akin to EAGAIN. Right now,
administrative requests are so short (copying and clearing a handful
of words out of PGPROC) that it's unlikely that this would be a
problem in practice.

> 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.

I feel mostly feel this way, too: I have a non-specific itch to solve
the race -- and I'm much more interested in pg_terminate_backend, the
original subject at hand (I'll submit a patch that is more of a
policy-fix than a signal wrangling one to the old thread).

I was also interested in sketching both as solution to this problem
and bringing down some of the non-intrinsic difficulty in
experimenting with rich inter-backend communication. For example,
query progress (itself a much more complex beast, obviously) could
probably make use of these administration requests (with an async-back
channel akin to NOTIFY). Or printing MemoryContextStats. Or quick
profiling toggling/performance counters on a backend run amok without
having the ability to run gdb -p and completely freezing the process.

Thoughts appreciated. I've marked the commitfest patch as "rejected"
for the time being, but if one is interested in peering at the work,
it is here:

https://commitfest.postgresql.org/action/patch_view?id=825

--
fdr

On Sun, Mar 18, 2012 at 01:28, Daniel Farina <daniel(at)heroku(dot)com> wrote:
> Noah offered me these comments:
>> 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.
>
> Is the postmaster signaling its children intrinsically vulnerable to
> PID racing?  Because it controls when it can call wait() or waitpid()
> on child processes, it can unambiguously know that PIDs have not been
> cycled for use.  For this reason, a credible and entirely alternate

As a note for future work, I don't think this assumption holds on
win32. We have a background thread there that picks up "child dead"
events, and posts those on an asynchronous queue (see
pgwin32_deadchild_callback).

And even if we didn't, I'm not sure the *process id* is "blocked"
until you wait on in. There is no "zombie state" for processes on
win32 - it dies, and the process handle becomes signaled (note that
this is also the process *handle*, and not the process id. There may
be multiple handles opened to the same process, but the process itself
goes away as soon as they are switched to signalled mode, even if
nobody was paying attention).

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

On Sat, Mar 17, 2012 at 05:28:11PM -0700, Daniel Farina wrote:
> Noah offered me these comments:
> > 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.
>
> Is the postmaster signaling its children intrinsically vulnerable to
> PID racing? Because it controls when it can call wait() or waitpid()
> on child processes, it can unambiguously know that PIDs have not been
> cycled for use.

Agreed, for Unix anyway.

> For this reason, a credible and entirely alternate
> design might be to bounce IPC requests through the postmaster, but
> since postmaster is so critical I had decided not to introduce nor
> change mechanics there.

Good point, but I also agree with your decision there.

> The Postmaster I think keeps a private copy of cancellation keys that
> are not in shared memory, if I read it properly (not 100% sure), and
> uses that for cancellation requests. This has a useful property of
> allowing cancellations even in event that shared memory goes insane
> (and since postmaster is typically left as last sane process of the
> group I thought it wise to not have it reuse a shared-memory based
> approach).

Yes.

> > 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.
>
> Yes. It could be fixed with dynamic allocation (holding more
> administration requests), but for just getting a flavor of what a
> solution might be like. I wanted to avoid additional dynamic
> allocation (which would necessitate a similar condition in the form of
> much-less likely OOM), but at some point I think this error condition
> is inevitable in some form. I see it as akin to EAGAIN. Right now,
> administrative requests are so short (copying and clearing a handful
> of words out of PGPROC) that it's unlikely that this would be a
> problem in practice.

I nominally agree that the new race would be rare, but not rarer than the race
this patch purposes to remove. You could also fix this by having the sender
wait until the target is ready to accept an admin request. For the particular
case of cancel/terminate, a terminate could overwrite a cancel; a cancel can
reduce to a no-op when either request is pending. I share your interest in
not tying a design to the narrow needs of cancel/terminate, though.

On Sat, Mar 17, 2012 at 8:28 PM, Daniel Farina <daniel(at)heroku(dot)com> wrote:
> This thread evolved out of an attempt to implement
> pg_terminate_backend for non-superusers.  I thought -- probably
> erroneously -- that the major objection to that was the known
> possibility of a PID-cycling race condition, whereby a signal could be
> misdirected, in the case of terminate_backend, SIGTERM.  So now this
> fork of the thread is about fixing these unlikely races, and then
> passing administration requests (such as "query cancel" or "die" ) as
> out-of-band information via SIGUSR1, just like how LISTEN/NOTIFY and
> conflict signals are passed.
>
> To prevent ambiguity, I am using a new special number -- a 'SessionId'
> -- that is guaranteed unique to all backends ever created during the
> uptime of a database.  This number is currently implemented in a way
> that is guessable (so it cannot be accepted from external sources),
> but I actually think it may be even more useful for a number of other
> uses if given a non-guessable form (like cancellation keys).  In this
> respect it would fulfill pretty much the same purposes as the notion
> of a "session" seen on the web.

It's after midnight here so maybe I'm being slow, but I don't
understand what problem the SessionId solves. ISTM that you could
solve the problem like this:

1. Acquire ProcArrayLock in exclusive mode, to keep the set of PGPROCs
from changing.
2. Search for the target PGPROC by PID; when you find it, set a bit in
the PGPROC indicating that you want it to cancel/die/whatever.
3. Remember the PID.
4. Release ProcArrayLock.
5. Send SIGUSR1.

If the PGPROC gets recycled between 4 and 5, the target backend will
find the relevant bits no longer set, and will do nothing.

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

On Mon, Mar 19, 2012 at 9:08 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> It's after midnight here so maybe I'm being slow, but I don't
> understand what problem the SessionId solves.  ISTM that you could
> solve the problem like this:
>
> 1. Acquire ProcArrayLock in exclusive mode, to keep the set of PGPROCs
> from changing.
> 2. Search for the target PGPROC by PID; when you find it, set a bit in
> the PGPROC indicating that you want it to cancel/die/whatever.
> 3. Remember the PID.
> 4. Release ProcArrayLock.
> 5. Send SIGUSR1.

I chose the SessionId mostly because I didn't have a great sense
around how hot the ProcArrayLock is, and it was easy to add a
fine-grained spinlock to just get the flavor of the idea.

To attempt to simplify your protocol more: is it necessary or sound to
remember the PID at all if one takes a lock on the entire PGPROC
array? At that point backend birth and death contends against that
lock, so the postmaster just has to initialize PGPROC the usual way
(writing over the administrative action words) and then...I don't see
a problem, on first blush. But I'll see your midnight and raise you a
1:35 AM.

I do think a session identifier will be useful someday in Postgres's
future, but it only really is worth it if it is non-guessable, to
which my implementation need not apply.

Also, I had a use case that would be roughly right for "cross-backend
administration" I want to temporarily suppress autovacuum/autoanalyze
without having to muck with thrashing configuration files, if
possible.

--
fdr

On Tue, Mar 20, 2012 at 4:35 AM, Daniel Farina <daniel(at)heroku(dot)com> wrote:
> I chose the SessionId mostly because I didn't have a great sense
> around how hot the ProcArrayLock is, and it was easy to add a
> fine-grained spinlock to just get the flavor of the idea.

It's fairly hot, but terminating or canceling backends is a
sufficiently rare operation that I don't think it matters.
Terminating a large number of backends this way all at once might be
enough to create a performance hit for concurrent activities, like a
pgbench -S run that's taking snapshots at full speed. But you should
recoup that cost in a fraction of a second from the fact that there
are now many fewer backends around - even if they were idle, they
still make the ProcArray larger, and if they weren't idle, even
moreso. So I think there's no problem there that's worth sweating
over.

> To attempt to simplify your protocol more: is it necessary or sound to
> remember the PID at all if one takes a lock on the entire PGPROC
> array?  At that point backend birth and death contends against that
> lock, so the postmaster just has to initialize PGPROC the usual way
> (writing over the administrative action words) and then...I don't see
> a problem, on first blush. But I'll see your midnight and raise you a
> 1:35 AM.

Well, I'm not sure it would save anything meaningful to read the PID
after releasing the lock even if it were safe, so I'd be inclined to
keep things simple. But on further reflection I had us using the PID
to find the target PGPROC in the first place, so we don't need to
"remember" a value that we already know; that step is simply
redundant.

> Also, I had a use case that would be roughly right for "cross-backend
> administration" I want to temporarily suppress autovacuum/autoanalyze
> without having to muck with thrashing configuration files, if
> possible.

ALTER TABLE can do this on a per-table basis. I think that creating a
separate way to control this on a system-wide basis would probably be
a mistake; it's rarely a good idea to have two separate systems to
manipulate the same behavior. I have often wished that we had an
ALTER SYSTEM command that could be used to make certain kinds of state
changes in lieu of having GUCs, because there are lots of things (e.g.
wal_level) that can only be changed at startup for want of the ability
to be certain that a config reload will result in everyone getting the
memo. A DDL command would have more flexibility in that regard. But
I wouldn't choose to use it for this unless it replaced, rather than
duplicated, the existing GUC.

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> Well, I'm not sure it would save anything meaningful to read the PID
> after releasing the lock even if it were safe, so I'd be inclined to
> keep things simple. But on further reflection I had us using the PID
> to find the target PGPROC in the first place, so we don't need to
> "remember" a value that we already know; that step is simply
> redundant.

I'm confused. If the premise is that PID is untrustworthy as a process
ID, how does searching PGPROC make it more trustworthy? The
hypothetical new owner of the PID could have gotten into PGPROC before
you begin to look.

What would make sense to me is to search PGPROC for some *other*
identifying property (and then set bit, remember PID, unlock, send
signal). But it seems like the key point here is what are we going
to use as an identifying property.

regards, tom lane

On Tue, Mar 20, 2012 at 12:48 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> Well, I'm not sure it would save anything meaningful to read the PID
>> after releasing the lock even if it were safe, so I'd be inclined to
>> keep things simple.  But on further reflection I had us using the PID
>> to find the target PGPROC in the first place, so we don't need to
>> "remember" a value that we already know; that step is simply
>> redundant.
>
> I'm confused.  If the premise is that PID is untrustworthy as a process
> ID, how does searching PGPROC make it more trustworthy?  The
> hypothetical new owner of the PID could have gotten into PGPROC before
> you begin to look.

Hmm, I guess that's true.

> What would make sense to me is to search PGPROC for some *other*
> identifying property (and then set bit, remember PID, unlock, send
> signal).  But it seems like the key point here is what are we going
> to use as an identifying property.

Well, Dan's idea of an ascending 64-bit sequence number would work,
but then we'd have to whack around the API to pg_cancel_backend and
pg_terminate_backend to accept that identifier in lieu of a PID, or
have alternate versions defined that way, and we'd have to export the
identifiers through pg_stat_activity as well.

Maybe we should just not worry about this.

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> Maybe we should just not worry about this.

That's been my reaction right along. There's no evidence that PID
recycling is a problem in the real world.

regards, tom lane

On Tue, Mar 20, 2012 at 10:13 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> Maybe we should just not worry about this.
>
> That's been my reaction right along.  There's no evidence that PID
> recycling is a problem in the real world.

I'm entirely willing to acquiesce to that point of view. I only
thought this was the blocker as to why pg_terminate_backend was left
out of the pg_cancel_backend patch.

In that thread, I have since posted the simpler "just let it happen" version.

--
fdr

On Tue, Mar 20, 2012 at 18:48, Daniel Farina <daniel(at)heroku(dot)com> wrote:
> On Tue, Mar 20, 2012 at 10:13 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>> Maybe we should just not worry about this.
>>
>> That's been my reaction right along.  There's no evidence that PID
>> recycling is a problem in the real world.
>
> I'm entirely willing to acquiesce to that point of view.  I only
> thought this was the blocker as to why pg_terminate_backend was left
> out of the pg_cancel_backend patch.

Late back into this thread.

I wasn't aware that was the reason there. I think it was the general
"leftovers" from previous times. When we first created
pg_terminate_backend() there was a general thought that it might not
be safe to just SIGTERM a backend to make it quit. A bunch of fixes
were put in place to make it more safe, but I'm not sure anybody
actually declared it fully safe. And I think it's a lot of legacy from
that time that just steers people towards the baby-steps approach.

I'm not sure - perhaps we're past that worry these days?

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

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> I wasn't aware that was the reason there. I think it was the general
> "leftovers" from previous times. When we first created
> pg_terminate_backend() there was a general thought that it might not
> be safe to just SIGTERM a backend to make it quit.

Not just "might not be safe" - it was demonstrably buggy in the
beginning.

> A bunch of fixes
> were put in place to make it more safe, but I'm not sure anybody
> actually declared it fully safe.

We never did, we only said we didn't know of more bugs.

> I'm not sure - perhaps we're past that worry these days?

I'm not. I still wouldn't trust SIGTERMing an individual backend in a
production database. It'll probably work, but what if it doesn't?
Best-case scenario is you'll need to do a panic shutdown to clear the
stuck lock or whatever that the backend left behind. (Once you've
diagnosed the problem, that is.) Now, in a case where the alternative
is a database shutdown anyway, you might as well try it. But it's the
kind of tool you only want to hand to responsible adults, which is why
it's superuser-only at the moment. I'm not sure we should be
encouraging people to fire that weapon indiscriminately.

regards, tom lane

On Mon, Mar 26, 2012 at 1:57 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> I'm not.  I still wouldn't trust SIGTERMing an individual backend in a
> production database.  It'll probably work, but what if it doesn't?
> Best-case scenario is you'll need to do a panic shutdown to clear the
> stuck lock or whatever that the backend left behind.  (Once you've
> diagnosed the problem, that is.)  Now, in a case where the alternative
> is a database shutdown anyway, you might as well try it.  But it's the
> kind of tool you only want to hand to responsible adults, which is why
> it's superuser-only at the moment.  I'm not sure we should be
> encouraging people to fire that weapon indiscriminately.

Okay, it was my precise intention to hand this to users so that not
only could they cancel their queries, but also force the transaction
to be aborted and the connection to be closed in case there is a
client run amok. Is there a good injection site -- perhaps
immediately after query cancellation -- where we can put in a
rollback-and-disconnect behavior?

Given this information, my understanding is that even the superuser is
coerced into undertaking an action that is unnecessarily dangerous if
they think the backend would respond to cancellation, but they also
wish to close the connection.

--
fdr

On Mon, Mar 26, 2012 at 4:57 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> I'm not sure - perhaps we're past that worry these days?
>
> I'm not.  I still wouldn't trust SIGTERMing an individual backend in a
> production database.  It'll probably work, but what if it doesn't?
> Best-case scenario is you'll need to do a panic shutdown to clear the
> stuck lock or whatever that the backend left behind.  (Once you've
> diagnosed the problem, that is.)  Now, in a case where the alternative
> is a database shutdown anyway, you might as well try it.  But it's the
> kind of tool you only want to hand to responsible adults, which is why
> it's superuser-only at the moment.  I'm not sure we should be
> encouraging people to fire that weapon indiscriminately.

I don't think we should be overly afraid of bugs in this code path. I
mean, there could very well be residual bugs, but that can be said of
anything. Moreover, if there are bugs, I'd like to find them and fix
them rather than living forever in a state of fear.

And frankly, if we're going to pick a feature to give the hairy
eyeball, this one wouldn't make my top ten list.

I think the more important question is a policy question: do we want
it to work like this? It seems like a policy question that ought to
be left to the DBA, but we have no policy management framework for
DBAs to configure what they do or do not wish to allow. Still, if
we've decided it's OK to allow cancelling, I don't see any real reason
why this should be treated differently.

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

Daniel Farina <daniel(at)heroku(dot)com> writes:
> On Mon, Mar 26, 2012 at 1:57 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> I'm not. I still wouldn't trust SIGTERMing an individual backend in a
>> production database.

> Okay, it was my precise intention to hand this to users so that not
> only could they cancel their queries, but also force the transaction
> to be aborted and the connection to be closed in case there is a
> client run amok. Is there a good injection site -- perhaps
> immediately after query cancellation -- where we can put in a
> rollback-and-disconnect behavior?

> Given this information, my understanding is that even the superuser is
> coerced into undertaking an action that is unnecessarily dangerous if
> they think the backend would respond to cancellation, but they also
> wish to close the connection.

Hrm, I think we're talking at cross-purposes here.

Me: "This mechanism hasn't been tested enough, and may still have nasty bugs."

You: "Then let's invent some entirely new mechanism."

I'm not seeing how that responds to the concern.

The original rationale for exposing pg_terminate_backend as a
superuser-only function was, as Magnus said, to take a baby step toward
providing the functionality generally. At this point we can either
think of another baby step, or cross our fingers and open the
floodgates. I'm just expressing discomfort with the second option.
I have to admit that I don't have a concrete proposal for a second baby
step (IOW an intermediate level of functionality that might provide
scope for more testing while not opening it up all the way yet).

regards, tom lane

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> I think the more important question is a policy question: do we want
> it to work like this? It seems like a policy question that ought to
> be left to the DBA, but we have no policy management framework for
> DBAs to configure what they do or do not wish to allow. Still, if
> we've decided it's OK to allow cancelling, I don't see any real reason
> why this should be treated differently.

Right now the only thing you can do to lock down pg_cancel_backend is
to make sure non-mutually-trusting users aren't given the same user ID.
Which, well, duh. Somebody with your user ID can probably do a lot more
damage than just cancelling your queries/sessions.

I do wonder though (and am too lazy to go look) whether the
pg_cancel_backend check is a strict user ID match or it allows
member-of-role matches. We might want to think a bit more carefully
about the implications if it's the latter.

regards, tom lane

On Tue, Mar 27, 2012 at 12:57 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Hrm, I think we're talking at cross-purposes here.
>
> Me: "This mechanism hasn't been tested enough, and may still have nasty bugs."
>
> You: "Then let's invent some entirely new mechanism."
>
> I'm not seeing how that responds to the concern.

I assume the intention was that the "entirely new mechanism" would be
a less risky one.

I may be forgetting something obvious here but is there even a
function to send an interrupt signal? That would trigger the same
behaviour that a user hitting C-c would trigger which would only be
handled at the next CHECK_FOR_INTERRUPTS which seems like it would be
non-controversial and iirc we don't currently have a function to do
this for other connections the user may have if he doesn't have access
to the original terminal and doesn't have raw shell access to run
arbitrary commands.

--
greg

On Tue, Mar 27, 2012 at 03:17:36AM +0100, Greg Stark wrote:
> I may be forgetting something obvious here but is there even a
> function to send an interrupt signal? That would trigger the same
> behaviour that a user hitting C-c would trigger which would only be
> handled at the next CHECK_FOR_INTERRUPTS which seems like it would be
> non-controversial and iirc we don't currently have a function to do
> this for other connections the user may have if he doesn't have access
> to the original terminal and doesn't have raw shell access to run
> arbitrary commands.

Sure, C-c just sends a SIGINT. But IIRC the problem wasn't so much
cancelling running queries, SIGINT appears to work fine there, it's
that a connection blocked on waiting for client input doesn't wake up
when sent a signal. To fix that you'd need to change the signal mode
and teach the various input layers (like SSL) to handle the EINTR case.

Have a nice day,
--
Martijn van Oosterhout <kleptog(at)svana(dot)org> http://svana.org/kleptog/
> He who writes carelessly confesses thereby at the very outset that he does
> not attach much importance to his own thoughts.
-- Arthur Schopenhauer

On Mon, Mar 26, 2012 at 07:53:25PM -0400, Robert Haas wrote:
> I think the more important question is a policy question: do we want
> it to work like this? It seems like a policy question that ought to
> be left to the DBA, but we have no policy management framework for
> DBAs to configure what they do or do not wish to allow. Still, if
> we've decided it's OK to allow cancelling, I don't see any real reason
> why this should be treated differently.

The DBA can customize policy by revoking public execute permissions on
pg_catalog.pg_terminate_backend and interposing a security definer function
implementing his checks. For the population who will want something different
here, that's adequate.

On Tue, Mar 27, 2012 at 11:04, Noah Misch <noah(at)leadboat(dot)com> wrote:
> On Mon, Mar 26, 2012 at 07:53:25PM -0400, Robert Haas wrote:
>> I think the more important question is a policy question: do we want
>> it to work like this?  It seems like a policy question that ought to
>> be left to the DBA, but we have no policy management framework for
>> DBAs to configure what they do or do not wish to allow.  Still, if
>> we've decided it's OK to allow cancelling, I don't see any real reason
>> why this should be treated differently.
>
> The DBA can customize policy by revoking public execute permissions on
> pg_catalog.pg_terminate_backend and interposing a security definer function
> implementing his checks.  For the population who will want something different
> here, that's adequate.

Well, by that argument, we can keep pg_terminate_backend superuser
only and have the user wrap a security definer function around it to
*get* it, no?

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

On Tue, Mar 27, 2012 at 02:58:26PM +0200, Magnus Hagander wrote:
> On Tue, Mar 27, 2012 at 11:04, Noah Misch <noah(at)leadboat(dot)com> wrote:
> > On Mon, Mar 26, 2012 at 07:53:25PM -0400, Robert Haas wrote:
> >> I think the more important question is a policy question: do we want
> >> it to work like this? ?It seems like a policy question that ought to
> >> be left to the DBA, but we have no policy management framework for
> >> DBAs to configure what they do or do not wish to allow. ?Still, if
> >> we've decided it's OK to allow cancelling, I don't see any real reason
> >> why this should be treated differently.
> >
> > The DBA can customize policy by revoking public execute permissions on
> > pg_catalog.pg_terminate_backend and interposing a security definer function
> > implementing his checks. ?For the population who will want something different
> > here, that's adequate.
>
> Well, by that argument, we can keep pg_terminate_backend superuser
> only and have the user wrap a security definer function around it to
> *get* it, no?

Yes. However, if letting users terminate their own backends makes for a
better default, we should still make it so.

Noah Misch <noah(at)leadboat(dot)com> writes:
> On Mon, Mar 26, 2012 at 07:53:25PM -0400, Robert Haas wrote:
>> I think the more important question is a policy question: do we want
>> it to work like this?

> The DBA can customize policy by revoking public execute permissions on
> pg_catalog.pg_terminate_backend and interposing a security definer function
> implementing his checks. For the population who will want something different
> here, that's adequate.

I don't particularly trust solutions that involve modifying
system-defined objects. In this case, a dump and reload would be
sufficient to create a security hole, because the REVOKE would go away.

(Now, I'm not particularly concerned about the issue in the first place.
Just pointing out that for someone who is, the above isn't a great
solution.)

regards, tom lane

On Mon, Mar 26, 2012 at 4:53 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> I think the more important question is a policy question: do we want
> it to work like this?  It seems like a policy question that ought to
> be left to the DBA, but we have no policy management framework for
> DBAs to configure what they do or do not wish to allow.  Still, if
> we've decided it's OK to allow cancelling, I don't see any real reason
> why this should be treated differently.

Is there a hypothetical DBA that doesn't want a mere-mortal user to be
able to signal one of their own backends to do "cancel query, rollback
the transaction, then close the socket"? If so, why?

--
fdr

On Tue, Mar 27, 2012 at 1:26 PM, Daniel Farina <daniel(at)heroku(dot)com> wrote:
> On Mon, Mar 26, 2012 at 4:53 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> I think the more important question is a policy question: do we want
>> it to work like this?  It seems like a policy question that ought to
>> be left to the DBA, but we have no policy management framework for
>> DBAs to configure what they do or do not wish to allow.  Still, if
>> we've decided it's OK to allow cancelling, I don't see any real reason
>> why this should be treated differently.
>
> Is there a hypothetical DBA that doesn't want a mere-mortal user to be
> able to signal one of their own backends to do "cancel query, rollback
> the transaction, then close the socket"?  If so, why?

Well, I guess if you have different people sharing the same user-ID,
you probably wouldn't want that.

But maybe that's not an important case.

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

Excerpts from Robert Haas's message of mar mar 27 14:38:47 -0300 2012:
> On Tue, Mar 27, 2012 at 1:26 PM, Daniel Farina <daniel(at)heroku(dot)com> wrote:
> > On Mon, Mar 26, 2012 at 4:53 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> >> I think the more important question is a policy question: do we want
> >> it to work like this?  It seems like a policy question that ought to
> >> be left to the DBA, but we have no policy management framework for
> >> DBAs to configure what they do or do not wish to allow.  Still, if
> >> we've decided it's OK to allow cancelling, I don't see any real reason
> >> why this should be treated differently.
> >
> > Is there a hypothetical DBA that doesn't want a mere-mortal user to be
> > able to signal one of their own backends to do "cancel query, rollback
> > the transaction, then close the socket"?  If so, why?
>
> Well, I guess if you have different people sharing the same user-ID,
> you probably wouldn't want that.
>
> But maybe that's not an important case.

Isn't it the case that many web applications run under some common
database user regardless of the underlying webapp user? I wouldn't say
that's an unimportant case. Granted, the webapp user wouldn't have
permission to run arbitrary queries in the first place.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Tue, Mar 27, 2012 at 1:46 PM, Alvaro Herrera
<alvherre(at)commandprompt(dot)com> wrote:
> Excerpts from Robert Haas's message of mar mar 27 14:38:47 -0300 2012:
>> On Tue, Mar 27, 2012 at 1:26 PM, Daniel Farina <daniel(at)heroku(dot)com> wrote:
>> > On Mon, Mar 26, 2012 at 4:53 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> >> I think the more important question is a policy question: do we want
>> >> it to work like this?  It seems like a policy question that ought to
>> >> be left to the DBA, but we have no policy management framework for
>> >> DBAs to configure what they do or do not wish to allow.  Still, if
>> >> we've decided it's OK to allow cancelling, I don't see any real reason
>> >> why this should be treated differently.
>> >
>> > Is there a hypothetical DBA that doesn't want a mere-mortal user to be
>> > able to signal one of their own backends to do "cancel query, rollback
>> > the transaction, then close the socket"?  If so, why?
>>
>> Well, I guess if you have different people sharing the same user-ID,
>> you probably wouldn't want that.
>>
>> But maybe that's not an important case.
>
> Isn't it the case that many web applications run under some common
> database user regardless of the underlying webapp user?

Yes.

> I wouldn't say
> that's an unimportant case.  Granted, the webapp user wouldn't have
> permission to run arbitrary queries in the first place.

Right. That's why I'm thinking maybe it doesn't matter.

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

On Tue, Mar 27, 2012 at 10:46 AM, Alvaro Herrera
<alvherre(at)commandprompt(dot)com> wrote:
> Isn't it the case that many web applications run under some common
> database user regardless of the underlying webapp user?  I wouldn't say
> that's an unimportant case.  Granted, the webapp user wouldn't have
> permission to run arbitrary queries in the first place.

I thought as we have cancel_backend implemented (which this is a small
extension of), the PGPROC roleid must be a spot-on match.

--
fdr

Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> Daniel Farina <daniel(at)heroku(dot)com> wrote:

>> Is there a hypothetical DBA that doesn't want a mere-mortal user
>> to be able to signal one of their own backends to do "cancel
>> query, rollback the transaction, then close the socket"? If so,
>> why?

Setting aside possible bugs in the mechanism, I have trouble
imagining a use-case where it would be undesirable to allow this.

> Well, I guess if you have different people sharing the same
> user-ID, you probably wouldn't want that.

As Tom pointed out, if there's another person sharing the user ID
you're using, and you don't trust them, their ability to cancel your
session is likely way down the list of concerns you should have.

-Kevin

On Tue, Mar 27, 2012 at 10:48 AM, Daniel Farina <daniel(at)heroku(dot)com> wrote:
> On Tue, Mar 27, 2012 at 10:46 AM, Alvaro Herrera
> <alvherre(at)commandprompt(dot)com> wrote:
>> Isn't it the case that many web applications run under some common
>> database user regardless of the underlying webapp user?  I wouldn't say
>> that's an unimportant case.  Granted, the webapp user wouldn't have
>> permission to run arbitrary queries in the first place.
>
> I thought as we have cancel_backend implemented (which this is a small
> extension of), the PGPROC roleid must be a spot-on match.

I read your email again. I thought common => meant "same base
roleid", not "the same roleid", so I thought role inheritance was
getting into this, which it isn't. Nevermind.

--
fdr

On Tuesday, March 27, 2012 07:51:59 PM Kevin Grittner wrote:
> > Well, I guess if you have different people sharing the same
> > user-ID, you probably wouldn't want that.
>
>
> As Tom pointed out, if there's another person sharing the user ID
> you're using, and you don't trust them, their ability to cancel your
> session is likely way down the list of concerns you should have.
Hm. I don't think that is an entirely valid argumentation. The same user could
have entirely different databases. They even could have distinct access
countrol via the clients ip.
I have seen the same cluster being used for prod/test instances at smaller
shops several times.

Whether thats a valid usecase I have no idea.

Andres

Andres Freund <andres(at)anarazel(dot)de> wrote:
> On Tuesday, March 27, 2012 07:51:59 PM Kevin Grittner wrote:
>>> Well, I guess if you have different people sharing the same
>>> user-ID, you probably wouldn't want that.
>>
>>
>> As Tom pointed out, if there's another person sharing the user ID
>> you're using, and you don't trust them, their ability to cancel
>> your session is likely way down the list of concerns you should
>> have.
> Hm. I don't think that is an entirely valid argumentation. The
> same user could have entirely different databases. They even could
> have distinct access countrol via the clients ip.
> I have seen the same cluster being used for prod/test instances at
> smaller shops several times.
>
> Whether thats a valid usecase I have no idea.

Well, that does sort of leave an arguable vulnerability. Should the
same user only be allowed to kill the process from a connection to
the same database?

-Kevin

On 03/27/2012 03:14 PM, Kevin Grittner wrote:
> Andres Freund<andres(at)anarazel(dot)de> wrote:
>> On Tuesday, March 27, 2012 07:51:59 PM Kevin Grittner wrote:
>>>> Well, I guess if you have different people sharing the same
>>>> user-ID, you probably wouldn't want that.
>>>
>>> As Tom pointed out, if there's another person sharing the user ID
>>> you're using, and you don't trust them, their ability to cancel
>>> your session is likely way down the list of concerns you should
>>> have.
>> Hm. I don't think that is an entirely valid argumentation. The
>> same user could have entirely different databases. They even could
>> have distinct access countrol via the clients ip.
>> I have seen the same cluster being used for prod/test instances at
>> smaller shops several times.
>>
>> Whether thats a valid usecase I have no idea.
>
> Well, that does sort of leave an arguable vulnerability. Should the
> same user only be allowed to kill the process from a connection to
> the same database?
>

It might be a reasonable restriction in theory, but I doubt it's much of
a security gain.

cheers

andrew

On Tue, Mar 27, 2012 at 1:30 PM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>> Well, that does sort of leave an arguable vulnerability.  Should the
>> same user only be allowed to kill the process from a connection to
>> the same database?
>>
>
> It might be a reasonable restriction in theory, but I doubt it's much of a
> security gain.

If this restriction makes anyone feel better, it doesn't bother/change
anything for me in the slightest (for both terminate and cancel), and
that way no interesting pg_hba.conf trickery will be broken, as far as
I can see.

--
fdr

"Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> writes:
> Andres Freund <andres(at)anarazel(dot)de> wrote:
>> On Tuesday, March 27, 2012 07:51:59 PM Kevin Grittner wrote:
>>> As Tom pointed out, if there's another person sharing the user ID
>>> you're using, and you don't trust them, their ability to cancel
>>> your session is likely way down the list of concerns you should
>>> have.

>> Hm. I don't think that is an entirely valid argumentation. The
>> same user could have entirely different databases. They even could
>> have distinct access countrol via the clients ip.
>> I have seen the same cluster being used for prod/test instances at
>> smaller shops several times.
>>
>> Whether thats a valid usecase I have no idea.

> Well, that does sort of leave an arguable vulnerability. Should the
> same user only be allowed to kill the process from a connection to
> the same database?

I don't see a lot of merit in this argument either. If joeseviltwin
can connect as joe to database A, he can also connect as joe to
database B in the same cluster, and then do whatever damage he wants.

Fundamentally, if two users are sharing the same userid, *they are the
same user* as far as Postgres is concerned. It's just silly to make
protection decisions on the assumption that they might not be.
If a DBA does not like the consequences of that, the solution is
obvious.

regards, tom lane

On Wed, Mar 28, 2012 at 10:02 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> "Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> writes:
>> Andres Freund <andres(at)anarazel(dot)de> wrote:
>>> On Tuesday, March 27, 2012 07:51:59 PM Kevin Grittner wrote:
>>>> As Tom pointed out, if there's another person sharing the user ID
>>>> you're using, and you don't trust them, their ability to cancel
>>>> your session is likely way down the list of concerns you should
>>>> have.
>
>>> Hm. I don't think that is an entirely valid argumentation. The
>>> same user could have entirely different databases. They even could
>>> have distinct access countrol via the clients ip.
>>> I have seen the same cluster being used for prod/test instances at
>>> smaller shops several times.
>>>
>>> Whether thats a valid usecase I have no idea.
>
>> Well, that does sort of leave an arguable vulnerability.  Should the
>> same user only be allowed to kill the process from a connection to
>> the same database?
>
> I don't see a lot of merit in this argument either.  If joeseviltwin
> can connect as joe to database A, he can also connect as joe to
> database B in the same cluster, and then do whatever damage he wants.
>
> Fundamentally, if two users are sharing the same userid, *they are the
> same user* as far as Postgres is concerned.  It's just silly to make
> protection decisions on the assumption that they might not be.
> If a DBA does not like the consequences of that, the solution is
> obvious.

I'm coming around to the point of view that we should just make
pg_terminate_backend()'s behavior consistent with pg_cancel_backend()
and call it good.

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> I'm coming around to the point of view that we should just make
> pg_terminate_backend()'s behavior consistent with pg_cancel_backend()
> and call it good.

Yeah, the issues that are really of concern are not ones that that
function in itself can address.

regards, tom lane