Cancel/Kill backend functions

Magnus Hagander wrote:

Per previous discussions, here are two functions to send INT and TERM
signals to other backends.They permit only INT and TERM, and permits
sending only to postgresql backends (as registered in pgstat).

Why does this depend on pgstat? ISTM it would be better to use the
per-backend PGPROC information, which is stored in shared memory.
Consider TransactionIdIsInProgress() for an example.

-Neil

Per previous discussions, here are two functions to send INT and TERM
signals to other backends.They permit only INT and TERM, and permits
sending only to postgresql backends (as registered in pgstat).

Why does this depend on pgstat? ISTM it would be better to use the
per-backend PGPROC information, which is stored in shared memory.
Consider TransactionIdIsInProgress() for an example.

I guess the main reason is that I didn't find how to do it. With that
pointer, I can probably redo it.

The other thought is that you're not going to have much use of this if
you don't have pgstat anyway - how are you going to find out which
backends actually exist?

But I could certainly give it a try to recode it on that code.

//Magnus

Magnus Hagander wrote:

The other thought is that you're not going to have much use of this if
you don't have pgstat anyway - how are you going to find out which
backends actually exist?

Uh, what about ps(1)?

-Neil

"Magnus Hagander" <mha@sollentuna.net> writes:

The other thought is that you're not going to have much use of this if
you don't have pgstat anyway - how are you going to find out which
backends actually exist?

ps, perhaps? Anyway I agree with Neil that it'd be better not to have a
dependency on code that may not be running, when there's no need.

regards, tom lane

The other thought is that you're not going to have much use

of this if

you don't have pgstat anyway - how are you going to find out which
backends actually exist?

Uh, what about ps(1)?

Well, if you ran run ps(1), then you can probably run kill(1) too. The
main point of this patch was to be able to do it remotely. Now that I
think of it, there are probably a bunch of times when you can see the
process information with ps but can't kill(1) it.

With this point and others, I will update the patch to use the other
method to determine if a PID is actualy a backend.

//Magnus

Okay, here is an updated patch. now uses IsBackendPid(), which is
closely modeled (read cut-and-pasted) from TransactionIdIsInProgress().

Since it's no longer a pgstat function, I moved it to "misc.c". Not 100%
that's the right place either, but it seemed like the best alternative.

//Magnus

Magnus, would you please resumbit this as a context diff?

---------------------------------------------------------------------------

Magnus Hagander wrote:

Okay, here is an updated patch. now uses IsBackendPid(), which is
closely modeled (read cut-and-pasted) from TransactionIdIsInProgress().

Since it's no longer a pgstat function, I moved it to "misc.c". Not 100%
that's the right place either, but it seemed like the best alternative.

//Magnus

-----Original Message-----
From: Neil Conway [mailto:neilc@samurai.com]
Sent: den 22 maj 2004 10:00
To: Magnus Hagander
Cc: pgsql-patches@postgresql.org
Subject: Re: [PATCHES] Cancel/Kill backend functions

Magnus Hagander wrote:

Per previous discussions, here are two functions to send INT and TERM
signals to other backends.They permit only INT and TERM, and permits
sending only to postgresql backends (as registered in pgstat).

Why does this depend on pgstat? ISTM it would be better to use the
per-backend PGPROC information, which is stored in shared memory.
Consider TransactionIdIsInProgress() for an example.

-Neil

Content-Description: termbackend.patch

[ Attachment, skipping... ]

---------------------------(end of broadcast)---------------------------
TIP 7: don't forget to increase your free space map settings

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Arrgh, when will I ever learn :-(

Attached.

//Magnus

On Thu, May 27, 2004 at 08:08:34PM +0200, Magnus Hagander wrote:

Magnus Hagander wrote:

Okay, here is an updated patch. now uses IsBackendPid(), which is
closely modeled (read cut-and-pasted) from
TransactionIdIsInProgress().

I wonder what can happen if a backend passes the IsBackendPid() test and
terminates just before the kill() signal? It should be pretty unlikely
but you could signal the wrong process ... shouldn't the SInvalLock be
held throughout the whole operation?

--
Alvaro Herrera (<alvherre[a]dcc.uchile.cl>)
"El n�mero de instalaciones de UNIX se ha elevado a 10,
y se espera que este n�mero aumente" (UPM, 1972)

Okay, here is an updated patch. now uses IsBackendPid(), which is
closely modeled (read cut-and-pasted) from
TransactionIdIsInProgress().

I wonder what can happen if a backend passes the
IsBackendPid() test and
terminates just before the kill() signal? It should be pretty unlikely
but you could signal the wrong process ... shouldn't the SInvalLock be
held throughout the whole operation?

You'd actually need to get a pid *reuse* during that short time.
Otherwise, you're just kill():ing a nonexistant process, which should be
no problem.

This is the same as issuing a "kill -INT <pid>" from the shell after
doing ps(1), which is basically what this function tries to emulate.
Should be no more dangerous than that.

Bottom line - while maybe slightly more correcet, not sure it's
necessary.

//Magnus

Magnus Hagander wrote:

You'd actually need to get a pid *reuse* during that short time.

That isn't so implausible on a system which assigns PIDs randomly.
Holding the SInvalLock doesn't remove the race condition, but it
makes it less likely to occur for essentially very little cost.

Bottom line - while maybe slightly more correcet, not sure it's
necessary.

IMHO it's worth doing.

-Neil

Neil Conway <neilc@samurai.com> writes:

Magnus Hagander wrote:

You'd actually need to get a pid *reuse* during that short time.

That isn't so implausible on a system which assigns PIDs randomly.
Holding the SInvalLock doesn't remove the race condition, but it
makes it less likely to occur for essentially very little cost.

Other than holding a fairly critical lock for an operation that will
take an unknown amount of time.

Since PG is not root, even if the PID has been reused, it's not possible
to kill some random process with this. You could only kill another
postgres process, and in practice this would more or less have to be a
new backend that had gotten reassigned the same PID just abandoned a
moment before by the one you intended to kill.

I don't think this is a very likely scenario. In fact, I suspect there
are interlocks in the kernel to prevent recycling a PID quite that
quickly, because otherwise practically *any* use of kill() would be too
hazardous to contemplate.

Bottom line - while maybe slightly more correcet, not sure it's
necessary.

IMHO it's worth doing.

I disagree.

regards, tom lane

On Fri, May 28, 2004 at 01:01:10AM -0400, Tom Lane wrote:

Neil Conway <neilc@samurai.com> writes:

Magnus Hagander wrote:

You'd actually need to get a pid *reuse* during that short time.

That isn't so implausible on a system which assigns PIDs randomly.
Holding the SInvalLock doesn't remove the race condition, but it
makes it less likely to occur for essentially very little cost.

Other than holding a fairly critical lock for an operation that will
take an unknown amount of time.

With this comment, I take it you'd disagree with my recoding of
TransactionIdIsCurrentTransactionId().

The current code has to scan only the xid's in each PGPROC struct.
However I had to rewrite it to peek at pg_subtrans, and this is done
while the SInvalLock is share-locked. pg_subtrans may need to do some
I/O to get the data, and there could be multiple queries, depending on
the nesting level.

I could write it to save the xid's in PGPROC in a first pass, then
release the SInvalLock, then look at pg_subtrans. But I think doing it
this way has a ("is a?") race condition.

--
Alvaro Herrera (<alvherre[a]dcc.uchile.cl>)
Bob [Floyd] used to say that he was planning to get a Ph.D. by the "green
stamp method," namely by saving envelopes addressed to him as 'Dr. Floyd'.
After collecting 500 such letters, he mused, a university somewhere in
Arizona would probably grant him a degree. (Don Knuth)

Alvaro Herrera <alvherre@dcc.uchile.cl> writes:

With this comment, I take it you'd disagree with my recoding of
TransactionIdIsCurrentTransactionId().

The current code has to scan only the xid's in each PGPROC struct.
However I had to rewrite it to peek at pg_subtrans, and this is done
while the SInvalLock is share-locked. pg_subtrans may need to do some
I/O to get the data, and there could be multiple queries, depending on
the nesting level.

You're right, I think that's really awful :-(

regards, tom lane

Alvaro Herrera <alvherre@dcc.uchile.cl> writes:

I could write it to save the xid's in PGPROC in a first pass, then
release the SInvalLock, then look at pg_subtrans. But I think doing it
this way has a ("is a?") race condition.

The way that would be technically correct is to *first* look in
pg_subtrans to resolve the xid up to a main xid, then look in PGPROC
to see if that main xact is still active. (You can return "no"
immediately if the sub-xact is aborted, but that would require yet
another probe into pg_clog, which might not be worth the trouble.)

Of course that's likely to be unpleasantly slow. Making it faster is an
exercise for the student ;-).

It's worth considering here that the normal case might very soon be that
most tuples are in fact modified by subtransactions. So I would not
advise optimizing on the assumption that you won't normally have to look
at pg_subtrans.

IIRC there was some discussion of keeping subtrans IDs up to some
limited nesting depth right in PGPROC. I'm not sure that would help a
whole lot (it helps with a positive answer, but not with a negative).

regards, tom lane

Patch applied. Thanks.

Not sure where to document them. I think we talked about this already.

I updated the system catalog version.

---------------------------------------------------------------------------

Magnus Hagander wrote:

Arrgh, when will I ever learn :-(

Attached.

//Magnus

-----Original Message-----
From: Bruce Momjian [mailto:pgman@candle.pha.pa.us]
Sent: den 26 maj 2004 20:50
To: Magnus Hagander
Cc: Neil Conway; pgsql-patches@postgresql.org
Subject: Re: [PATCHES] Cancel/Kill backend functions

Magnus, would you please resumbit this as a context diff?

---------------------------------------------------------------
------------

Magnus Hagander wrote:

Okay, here is an updated patch. now uses IsBackendPid(), which is
closely modeled (read cut-and-pasted) from
TransactionIdIsInProgress().

Since it's no longer a pgstat function, I moved it to "misc.c". Not
100% that's the right place either, but it seemed like the best
alternative.

//Magnus

-----Original Message-----
From: Neil Conway [mailto:neilc@samurai.com]
Sent: den 22 maj 2004 10:00
To: Magnus Hagander
Cc: pgsql-patches@postgresql.org
Subject: Re: [PATCHES] Cancel/Kill backend functions

Magnus Hagander wrote:

Per previous discussions, here are two functions to send INT and
TERM signals to other backends.They permit only INT and TERM, and
permits sending only to postgresql backends (as registered in
pgstat).

Why does this depend on pgstat? ISTM it would be better to use the
per-backend PGPROC information, which is stored in shared memory.
Consider TransactionIdIsInProgress() for an example.

-Neil

Content-Description: termbackend.patch

[ Attachment, skipping... ]

---------------------------(end of
broadcast)---------------------------
TIP 7: don't forget to increase your free space map settings

-- 
Bruce Momjian                        |  http://candle.pha.pa.us
pgman@candle.pha.pa.us               |  (610) 359-1001
+  If your life is a hard drive,     |  13 Roberts Road
+  Christ can be your backup.        |  Newtown Square, 
Pennsylvania 19073

Content-Description: termbackend2.patch

[ Attachment, skipping... ]

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073