Thread: Cancel/Kill backend functions
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). Documentation to follow. I'd appreciate some pointers as to where to put this. A new section "Managing Connections" under "Server Administration" seems like perhaps a good place to put it, but might be overkill? //Magnus
Attachment
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 >-----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 >
Attachment
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 >-----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 >
Attachment
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