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

[Martijn van Oosterhout]

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   klep...@svana.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


signature.asc
Description: Digital signature

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

[Greg Stark]

On Tue, Mar 27, 2012 at 12:57 AM, Tom Lane t...@sss.pgh.pa.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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Noah Misch]

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.

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers