Обсуждение: pg_terminate_backend idea

I had an idea about how to possibly solve the pg_terminate_backend
issue.

How about we add a field to PGPROC (or is there a better place?) called
shouldExit. pg_terminate_backend will set this field to "true" in the
target backend, and then send the normal "cancel query" signal.

The receiving backend will check for this flag in PostgresMain, and
perform a proc_exit(0) if it is set.

This way, the net effect of doing pg_terminate_backend() will be that of
query cancel followed by the "lost connection to client" path. Both of
which should be well tested by now.

I hacked up a really quick test, and it seems to be working in theory.
But it still requires me to send some data (such as a dummy query) to
the backend before it exits. This is because server side libpq blocks
when reading and ignores signals at this time. I believe the fix for
this would be to pass a flag down to the libpq routines that we want to
be abort in case of signal+flag, set only when doing the "main call" to
recv, so we can kill idle process.

But I naturally wanted to float the idea here before digging in with
further coding. Does this idea have any merit, or does it just seem
stupid? ;-) (hey, if it is, at least it was a try..)


//Magnus

"Magnus Hagander" <mha@sollentuna.net> writes:
> But it still requires me to send some data (such as a dummy query) to
> the backend before it exits. This is because server side libpq blocks
> when reading and ignores signals at this time. I believe the fix for
> this would be to pass a flag down to the libpq routines that we want to
> be abort in case of signal+flag, set only when doing the "main call" to
> recv, so we can kill idle process.

Yech!  That code is messy enough already, lets not pile another kluge
atop it in order to handle something that's not even being requested
AFAIR.

In any case the correct way to solve the problem is to find out what's
being left corrupt by SIGTERM, rather than install more messiness in
order to avoid facing the real issue ...
        regards, tom lane

Tom Lane wrote:
> "Magnus Hagander" <mha@sollentuna.net> writes:
> 
>>But it still requires me to send some data (such as a dummy query) to
>>the backend before it exits. This is because server side libpq blocks
>>when reading and ignores signals at this time. I believe the fix for
>>this would be to pass a flag down to the libpq routines that we want to
>>be abort in case of signal+flag, set only when doing the "main call" to
>>recv, so we can kill idle process.
> 
> 
> Yech!  That code is messy enough already, lets not pile another kluge
> atop it in order to handle something that's not even being requested
> AFAIR.

I ran into the same problem back when I was trying to implement an
idle-in-transaction timeout, so solving this might be useful in more
than one place..

-O

Tom Lane wrote:
> "Magnus Hagander" <mha@sollentuna.net> writes:
> > But it still requires me to send some data (such as a dummy query) to
> > the backend before it exits. This is because server side libpq blocks
> > when reading and ignores signals at this time. I believe the fix for
> > this would be to pass a flag down to the libpq routines that we want to
> > be abort in case of signal+flag, set only when doing the "main call" to
> > recv, so we can kill idle process.
> 
> Yech!  That code is messy enough already, lets not pile another kluge
> atop it in order to handle something that's not even being requested
> AFAIR.
> 
> In any case the correct way to solve the problem is to find out what's
> being left corrupt by SIGTERM, rather than install more messiness in
> order to avoid facing the real issue ...

I am confused.  Are you talking about the client SIGTERM or the server? 
I thought we agreed that using the cancel functionality, which we know
works and is tested, to do backend terminate was the right approach. 
TODO has:
* Allow administrators to safely terminate individual sessions  Right now, SIGTERM will terminate a session, but it is
treatedas  though the postmaster has paniced and shared memory might not be  cleaned up properly.  A new signal is
neededfor safe termination  because backends must first do a query cancel, then exit once they  have run the query
cancelcleanup routine.
 

I don't see us ever able to handle backend terminate in any other way. 
Are you complaining about the issue with terminating the client?  I had
not considered that.

--  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,
Pennsylvania19073
 

Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Tom Lane wrote:
>> In any case the correct way to solve the problem is to find out what's
>> being left corrupt by SIGTERM, rather than install more messiness in
>> order to avoid facing the real issue ...

> I am confused.  Are you talking about the client SIGTERM or the server? 

I am talking about Rod Taylor's reports that SIGTERM'ing individual
backends tends to lead to "lock table corrupted" crashes awhile later.
Now, I've been playing the part of Chicken Little on this for awhile,
but seeing an actual report of problems from the field certainly
strengthens my feelings about it.

What I think we need to do is find a way to isolate and fix the behavior
Rod is seeing.  It may be that the bug occurs only for SIGTERM, or it
may be that it's a general problem that a SIGTERM just increases the
probability of seeing.  In any case I think we have to solve it, not
create new mechanisms to try to ignore it.

> TODO has:

>     * Allow administrators to safely terminate individual sessions
>       Right now, SIGTERM will terminate a session, but it is treated as
>       though the postmaster has paniced and shared memory might not be
>       cleaned up properly.

That statement is entirely incorrect.
        regards, tom lane

On Tue, 2005-06-21 at 23:34 -0400, Tom Lane wrote:
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > Tom Lane wrote:
> >> In any case the correct way to solve the problem is to find out what's
> >> being left corrupt by SIGTERM, rather than install more messiness in
> >> order to avoid facing the real issue ...
> 
> > I am confused.  Are you talking about the client SIGTERM or the server? 
> 
> I am talking about Rod Taylor's reports that SIGTERM'ing individual
> backends tends to lead to "lock table corrupted" crashes awhile later.
> Now, I've been playing the part of Chicken Little on this for awhile,
> but seeing an actual report of problems from the field certainly
> strengthens my feelings about it.
> 
> What I think we need to do is find a way to isolate and fix the behavior
> Rod is seeing.  It may be that the bug occurs only for SIGTERM, or it
> may be that it's a general problem that a SIGTERM just increases the
> probability of seeing.  In any case I think we have to solve it, not
> create new mechanisms to try to ignore it.

If it helps, it seems to occur primarily (perhaps always) when there are
schema changes being performed when the SIGTERM is issued.

I don't remember seeing them on Intel or on v7.2 (we didn't stay on 7.4
very long), but on a fairly well loaded Solaris machine (v880 with
between 100 and 200 connections) it happens enough that we automatically
schedule a server restart during the first opportunity when we need to
kill connections in this way This is generally when the server doesn't
recognize the client has dropped -- pgpool can be clumsy with
connections).

> > TODO has:
> 
> >     * Allow administrators to safely terminate individual sessions
>     
> >       Right now, SIGTERM will terminate a session, but it is treated as
> >       though the postmaster has paniced and shared memory might not be
> >       cleaned up properly.
> 
> That statement is entirely incorrect.
> 
>             regards, tom lane
> 
> ---------------------------(end of broadcast)---------------------------
> TIP 1: subscribe and unsubscribe commands go to majordomo@postgresql.org
> 
-- 

> > But it still requires me to send some data (such as a dummy
> query) to
> > the backend before it exits. This is because server side
> libpq blocks
> > when reading and ignores signals at this time. I believe
> the fix for
> > this would be to pass a flag down to the libpq routines
> that we want
> > to be abort in case of signal+flag, set only when doing the "main
> > call" to recv, so we can kill idle process.
>
> Yech!  That code is messy enough already, lets not pile
> another kluge atop it in order to handle something that's not
> even being requested AFAIR.

While I agree it'sa  bit of a cludge, saying that it's not requested is
absolutely and totally untrue. It has been requested *a lot*. People
even use a method that is now *known* to be unsafe, simply because we do
not provide another alternative.

Therefor, I prefer a kludge than nothing at all. But a "proper solution"
is of course better.


> In any case the correct way to solve the problem is to find
> out what's being left corrupt by SIGTERM, rather than install
> more messiness in order to avoid facing the real issue ...

That is unfortunatly way over my head. And it doesn't seem like anybody
who actually has what it takes to do the "proper solution" is interested
in doing it.


//Magnus

Bruce Momjian wrote:
> Tom Lane wrote:
> 
>>"Magnus Hagander" <mha@sollentuna.net> writes:
>>
>>>But it still requires me to send some data (such as a dummy query) to
>>>the backend before it exits. This is because server side libpq blocks
>>>when reading and ignores signals at this time. I believe the fix for
>>>this would be to pass a flag down to the libpq routines that we want to
>>>be abort in case of signal+flag, set only when doing the "main call" to
>>>recv, so we can kill idle process.
>>
>>Yech!  That code is messy enough already, lets not pile another kluge
>>atop it in order to handle something that's not even being requested
>>AFAIR.
>>
>>In any case the correct way to solve the problem is to find out what's
>>being left corrupt by SIGTERM, rather than install more messiness in
>>order to avoid facing the real issue ...
> 
> 
> I am confused.  Are you talking about the client SIGTERM or the server? 
> I thought we agreed that using the cancel functionality, which we know
> works and is tested,

I've seen cancel *not* working. In 80 % this was the reason to use 
terminate.

Regards,
Andreas

"Magnus Hagander" <mha@sollentuna.net> writes:
>> In any case the correct way to solve the problem is to find 
>> out what's being left corrupt by SIGTERM, rather than install 
>> more messiness in order to avoid facing the real issue ...

> That is unfortunatly way over my head. And it doesn't seem like anybody
> who actually has what it takes to do the "proper solution" is interested
> in doing it.

A test case --- even one that fails only a small percentage of the time
--- would make things far easier.  So far all I've seen are very vague
reports, and it's impossible to do anything about it without more info.
        regards, tom lane

Andreas Pflug <pgadmin@pse-consulting.de> writes:
>> I thought we agreed that using the cancel functionality, which we know
>> works and is tested,

> I've seen cancel *not* working. In 80 % this was the reason to use 
> terminate.

Even a moment's perusal of the code will prove that there is no
situation in which a backend will respond to SIGTERM but not SIGINT
--- there is only one InterruptPending flag and both cases are checked
in ProcessInterrupts().  So I don't believe the above argument for
using terminate in the slightest.

I can easily believe that we have missed some places that need a
CHECK_FOR_INTERRUPTS() call added, to ensure the backend can't go too
long without making these checks.  I added one in the planner main
loop just a couple weeks ago, for instance.  If you can identify what
a backend that's ignoring a cancel request is doing, please let us know.
        regards, tom lane

On 2005-06-22, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Andreas Pflug <pgadmin@pse-consulting.de> writes:
>>> I thought we agreed that using the cancel functionality, which we know
>>> works and is tested,
>
>> I've seen cancel *not* working. In 80 % this was the reason to use 
>> terminate.
>
> Even a moment's perusal of the code will prove that there is no
> situation in which a backend will respond to SIGTERM but not SIGINT

"idle in transaction". (or "idle" for that matter, but that's usually less
significant.)

-- 
Andrew, Supernews
http://www.supernews.com - individual and corporate NNTP services

Andrew - Supernews <andrew+nonews@supernews.com> writes:
> On 2005-06-22, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Andreas Pflug <pgadmin@pse-consulting.de> writes:
>>> I've seen cancel *not* working.
>> 
>> Even a moment's perusal of the code will prove that there is no
>> situation in which a backend will respond to SIGTERM but not SIGINT

> "idle in transaction". (or "idle" for that matter, but that's usually less
> significant.)

In that case there's no query to cancel, so I would dispute the claim
that that constitutes "not working".  QueryCancel is defined to cancel
the current query, not necessarily to abort your whole transaction.
(Before 8.0 there wasn't much of a difference, but now there is:
QueryCancel is an ordinary error that can be trapped by a savepoint.
Are you arguing it should not be so trappable?)
        regards, tom lane

On 2005-06-22, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Andrew - Supernews <andrew+nonews@supernews.com> writes:
>> On 2005-06-22, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Andreas Pflug <pgadmin@pse-consulting.de> writes:
>>>> I've seen cancel *not* working.
>>> 
>>> Even a moment's perusal of the code will prove that there is no
>>> situation in which a backend will respond to SIGTERM but not SIGINT
>
>> "idle in transaction". (or "idle" for that matter, but that's usually less
>> significant.)
>
> In that case there's no query to cancel, so I would dispute the claim
> that that constitutes "not working".

You are totally missing the point.

A backend that is "idle in transaction" is holding locks and an open xid
that cannot be cleared by anything short of SIGTERM.

Whether the fact that it ignores SIGINT is intentional or not is irrelevent,
the fact is that this is the classic scenario where SIGTERM is effective and
SIGINT is not.

-- 
Andrew, Supernews
http://www.supernews.com - individual and corporate NNTP services

> >> In any case the correct way to solve the problem is to find out
> >> what's being left corrupt by SIGTERM, rather than install more
> >> messiness in order to avoid facing the real issue ...
>
> > That is unfortunatly way over my head. And it doesn't seem like
> > anybody who actually has what it takes to do the "proper
> solution" is
> > interested in doing it.
>
> A test case --- even one that fails only a small percentage
> of the time
> --- would make things far easier.  So far all I've seen are
> very vague reports, and it's impossible to do anything about
> it without more info.

Very well. Let me try putting it like this, then:

Assuming we don't get such a case, and a chance to fix it, before 8.1
(while still hoping we will get it fixed properly, we can't be sure, can
we? If we were, it'd be fixed already). In this case, will you consider
such a kludgy solution as a temporary fix to resolve a problem that a
lot of users are having? And then plan to have it removed once sending
SIGTERM directly to a backend can be considered safe?


//Magnus

"Magnus Hagander" <mha@sollentuna.net> writes:
> Assuming we don't get such a case, and a chance to fix it, before 8.1
> (while still hoping we will get it fixed properly, we can't be sure, can
> we? If we were, it'd be fixed already). In this case, will you consider
> such a kludgy solution as a temporary fix to resolve a problem that a
> lot of users are having? And then plan to have it removed once sending
> SIGTERM directly to a backend can be considered safe?

Kluges tend to become institutionalized, so my reaction is "no".  It's
also worth pointing out that with so little understanding of the problem
Rod is reporting, it's tough to make a convincing case that this kluge
will avoid it.  SIGTERM exit *shouldn't* be leaving any corrupted
locktable entries behind; it's not that much different from the normal
case.  Until we find out what's going on, introducing still another exit
path isn't really going to make me feel more comfortable, no matter how
close it's alleged to be to the normal path.
        regards, tom lane

Tom Lane wrote:
> "Magnus Hagander" <mha@sollentuna.net> writes:
> > Assuming we don't get such a case, and a chance to fix it, before 8.1
> > (while still hoping we will get it fixed properly, we can't be sure, can
> > we? If we were, it'd be fixed already). In this case, will you consider
> > such a kludgy solution as a temporary fix to resolve a problem that a
> > lot of users are having? And then plan to have it removed once sending
> > SIGTERM directly to a backend can be considered safe?
>
> Kluges tend to become institutionalized, so my reaction is "no".  It's
> also worth pointing out that with so little understanding of the problem
> Rod is reporting, it's tough to make a convincing case that this kluge
> will avoid it.  SIGTERM exit *shouldn't* be leaving any corrupted
> locktable entries behind; it's not that much different from the normal
> case.  Until we find out what's going on, introducing still another exit
> path isn't really going to make me feel more comfortable, no matter how
> close it's alleged to be to the normal path.

I have been running some tests to try to see the lock table corruption
but I have been unable to reproduce the problem.  I have attached my
crude test scripts.  I would run the scripts and then open another
session as a different user and do UPDATE and LOCK to cause the psql
session to block.

The only functional difference I can see between a SIGTERM exit and a
cancel followed by a normal exit is the call to
AbortCurrentTransaction().  Could that be significant?  Because I can't
reproduce the failure I can't know for sure.

--
  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
#!/usr/contrib/bin/expect --
set timeout -1
eval spawn sql test
expect -nocase "test=>"
send "begin;\r"
expect -nocase "test=>"
send "lock pg_class;\r"
expect -nocase "test=>"
send "select * from pg_locks;\r"
expect -nocase "test=>"
send "update test set x=3;\r"
expect -nocase "test=>"
expect eof
exit
while :
do
    XPID=`/letc/ps_sysv -ef | grep 'postgres test'|grep -v grep|awk '{print $2}'`
    if [ "$XPID" != "" ]
    then    kill $XPID
        echo $XPID
        XPID=`/letc/ps_sysv -ef | grep 'psql test'|grep -v execargs|awk '{print $2}'`
        kill $XPID
    fi
    sleep 1
done

Bruce Momjian <pgman@candle.pha.pa.us> writes:
> I have been running some tests to try to see the lock table corruption
> but I have been unable to reproduce the problem.

It's possible that what Rod was complaining of is fixed in CVS tip ---
see discussion about RemoveFromWaitQueue() bug.  If so, it would have
been a bug that could be seen in other circumstances too, but maybe
SIGTERM made it more probable for some reason.
        regards, tom lane

Tom Lane wrote:
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > I have been running some tests to try to see the lock table corruption
> > but I have been unable to reproduce the problem.
> 
> It's possible that what Rod was complaining of is fixed in CVS tip ---
> see discussion about RemoveFromWaitQueue() bug.  If so, it would have
> been a bug that could be seen in other circumstances too, but maybe
> SIGTERM made it more probable for some reason.

Was that backpatched to 8.0.X?  If not, I can test that branch of CVS
for the problem.

--  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,
Pennsylvania19073
 

On Tue, 2005-06-21 at 23:34 -0400, Tom Lane wrote:
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > Tom Lane wrote:
> >> In any case the correct way to solve the problem is to find out what's
> >> being left corrupt by SIGTERM, rather than install more messiness in
> >> order to avoid facing the real issue ...
> 
> > I am confused.  Are you talking about the client SIGTERM or the server? 
> 
> I am talking about Rod Taylor's reports that SIGTERM'ing individual
> backends tends to lead to "lock table corrupted" crashes awhile later.
> Now, I've been playing the part of Chicken Little on this for awhile,
> but seeing an actual report of problems from the field certainly
> strengthens my feelings about it.

Bringing this thread back to life.

I have not seen a lock table corruption issue with SIGTERM in 8.1 on
Solaris/Sun IV, Linux/AMD64, or Linux/Intel. I don't recall seeing one
on 8.0.3 either though I'm pretty sure there were several on 8.0.1.

There are times when locks for a process hang around for a few minutes
before getting cleared. I don't recall whether they were ungranted table
locks or entries waiting on a transaction ID lock, but the source was
Slony and a large pg_listener structure with more than 20000 pages (yes,
pages not tuples).

I have also seen processes refusing to acknowledge the signal and exit
during btree index builds, but that's not a data corruption issue.
--