Thread: Queries that should be canceled will get stuck on secure_write function

On Mon, Aug 23, 2021 at 4:15 AM 蔡梦娟(玊于) <mengjuan.cmj@alibaba-inc.com> wrote:
> I want to know why the interrupt is only handled when ProcDiePending is true, I think query which is supposed to be
canceledalso should respond to the signal. 

Well, if we're halfway through sending a message to the client and we
abort the write, we have no way of re-establishing protocol sync,
right? It's OK to die without sending any more data to the client,
since then the connection is closed anyway and the loss of sync
doesn't matter, but continuing the session doesn't seem workable.

Your proposed patch actually seems to dodge this problem and I think
perhaps we could consider something along those lines. It would be
interesting to hear what Andres thinks about this idea, since I think
he was the last one to rewrite that code.

--
Robert Haas
EDB: http://www.enterprisedb.com

On 2021-Aug-23, Robert Haas wrote:

> On Mon, Aug 23, 2021 at 4:15 AM 蔡梦娟(玊于) <mengjuan.cmj@alibaba-inc.com> wrote:
> > I want to know why the interrupt is only handled when ProcDiePending
> > is true, I think query which is supposed to be canceled also should
> > respond to the signal.

Yeah, I agree.

> Well, if we're halfway through sending a message to the client and we
> abort the write, we have no way of re-establishing protocol sync,
> right? It's OK to die without sending any more data to the client,
> since then the connection is closed anyway and the loss of sync
> doesn't matter, but continuing the session doesn't seem workable.
> 
> Your proposed patch actually seems to dodge this problem and I think
> perhaps we could consider something along those lines.

Do we actually need new GUCs, though?  I think we should never let an
unresponsive client dictate what the server does, because that opens the
door for uncooperative or malicious clients to wreak serious havoc.  I
think the implementation should wait until time now+X to cancel the
query, but if by time now+2X (or whatever we deem reasonable -- maybe
now+1.1X) we're still waiting, then it's okay to just close the
connection.  This suggests a completely different implementation, though.

I wonder if it's possible to write a test for this.  We would have to
send a query and then hang the client somehow.  I recently added a TAP
test that uses SIGSTOP to a walsender ... can we use SIGSTOP with a
background psql that's running SELECT pg_sleep() perhaps?
(Or maybe it's sufficient to start background psql and not pump() it?)

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"It takes less than 2 seconds to get to 78% complete; that's a good sign.
A few seconds later it's at 90%, but it seems to have stuck there.  Did
somebody make percentages logarithmic while I wasn't looking?"
                http://smylers.hates-software.com/2005/09/08/1995c749.html

On Mon, Aug 23, 2021 at 10:45 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> Do we actually need new GUCs, though?  I think we should never let an
> unresponsive client dictate what the server does, because that opens the
> door for uncooperative or malicious clients to wreak serious havoc.  I
> think the implementation should wait until time now+X to cancel the
> query, but if by time now+2X (or whatever we deem reasonable -- maybe
> now+1.1X) we're still waiting, then it's okay to just close the
> connection.  This suggests a completely different implementation, though.

I don't quite understand what you mean by waiting until time now+X to
cancel the query. We don't know a priori when query cancels are going
to happen, but when they do happen we want to respond to them as
quickly as we can. It seems reasonable to say that if we can't handle
them within X amount of time we can resort to emergency measures, but
that's basically what the patch does, except it uses a GUC instead of
hardcoding X.

> I wonder if it's possible to write a test for this.  We would have to
> send a query and then hang the client somehow.  I recently added a TAP
> test that uses SIGSTOP to a walsender ... can we use SIGSTOP with a
> background psql that's running SELECT pg_sleep() perhaps?
> (Or maybe it's sufficient to start background psql and not pump() it?)

Starting a background process and not pumping it sounds promising,
because it seems like it would be more likely to be portable. I think
we'd want to be careful not to assume very much about how large the
output buffer is, because I'm guessing that varies by platform and
that it might in some cases be fairly large. Perhaps we could use
pg_stat_activity to wait until we block in a ClientWrite state,
although I wonder if we might find out that we sometimes block on a
different wait state than what we expect to see.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

On 2021-Aug-23, Robert Haas wrote:

> On Mon, Aug 23, 2021 at 10:45 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > Do we actually need new GUCs, though?  I think we should never let an
> > unresponsive client dictate what the server does, because that opens the
> > door for uncooperative or malicious clients to wreak serious havoc.  I
> > think the implementation should wait until time now+X to cancel the
> > query, but if by time now+2X (or whatever we deem reasonable -- maybe
> > now+1.1X) we're still waiting, then it's okay to just close the
> > connection.  This suggests a completely different implementation, though.
> 
> I don't quite understand what you mean by waiting until time now+X to
> cancel the query. We don't know a priori when query cancels are going
> to happen, but when they do happen we want to respond to them as
> quickly as we can. It seems reasonable to say that if we can't handle
> them within X amount of time we can resort to emergency measures, but
> that's basically what the patch does, except it uses a GUC instead of
> hardcoding X.

Aren't we talking about query cancellations that occur in response to a
standby delay limit?  Those aren't in response to user action.  What I
mean is that if the standby delay limit is exceeded, then we send a
query cancel; we expect the standby to cancel its query at that time and
then the primary can move on.  But if the standby doesn't react, then we
can have it terminate its connection.  I'm looking at the problem from
the primary's point of view rather than the standby's point of view.

-- 
Álvaro Herrera              Valdivia, Chile  —  https://www.EnterpriseDB.com/

On Mon, Aug 23, 2021 at 11:26 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> Aren't we talking about query cancellations that occur in response to a
> standby delay limit?  Those aren't in response to user action.

Oh, you're right. But I guess a similar problem could also occur in
response to pg_terminate_backend(), no?

-- 
Robert Haas
EDB: http://www.enterprisedb.com

On 2021/08/24 0:26, Alvaro Herrera wrote:
> Aren't we talking about query cancellations that occur in response to a
> standby delay limit?  Those aren't in response to user action.  What I
> mean is that if the standby delay limit is exceeded, then we send a
> query cancel; we expect the standby to cancel its query at that time and
> then the primary can move on.  But if the standby doesn't react, then we
> can have it terminate its connection.

+1


On 2021/08/24 3:45, Robert Haas wrote:
> On Mon, Aug 23, 2021 at 11:26 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>> Aren't we talking about query cancellations that occur in response to a
>> standby delay limit?  Those aren't in response to user action.
> 
> Oh, you're right. But I guess a similar problem could also occur in
> response to pg_terminate_backend(), no?

There seems no problem in that case because pg_terminate_backend() causes
a backend to set ProcDiePending to true in die() signal hander and
ProcessClientWriteInterrupt() called by secure_write() handles ProcDiePending.
No?

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On Tue, Aug 24, 2021 at 1:15 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> > Oh, you're right. But I guess a similar problem could also occur in
> > response to pg_terminate_backend(), no?
>
> There seems no problem in that case because pg_terminate_backend() causes
> a backend to set ProcDiePending to true in die() signal hander and
> ProcessClientWriteInterrupt() called by secure_write() handles ProcDiePending.
> No?

Hmm, maybe you're right. What about pg_cancel_backend()?

-- 
Robert Haas
EDB: http://www.enterprisedb.com

On 2021/08/25 2:30, Robert Haas wrote:
> Hmm, maybe you're right. What about pg_cancel_backend()?

I was thinking that it's valid even if secure_write() doesn't react to
pg_cancel_backend() because it's basically called outside transaction block,
i.e., there is no longer transaction to cancel in that case. But there can be
some cases where secure_write() is called inside transaction block,
for example, when the query generates NOTICE message. In these cases,
secure_write() might need to react to the cancel request.

BTW, when an error happens, I found that a backend calls EmitErrorReport()
to report an error to a client, and then calls AbortCurrentTransaction()
to abort the transaction. If secure_write() called by EmitErrorReport()
gets stuck, a backend gets stuck inside transaction block and the locks
keep being held unnecessarily. Isn't this problematic? Can we change
the order of them?

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On Tue, Aug 24, 2021 at 9:58 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> I was thinking that it's valid even if secure_write() doesn't react to
> pg_cancel_backend() because it's basically called outside transaction block,
> i.e., there is no longer transaction to cancel in that case. But there can be
> some cases where secure_write() is called inside transaction block,
> for example, when the query generates NOTICE message. In these cases,
> secure_write() might need to react to the cancel request.

Yeah. I think we could also be sending tuple data.

> BTW, when an error happens, I found that a backend calls EmitErrorReport()
> to report an error to a client, and then calls AbortCurrentTransaction()
> to abort the transaction. If secure_write() called by EmitErrorReport()
> gets stuck, a backend gets stuck inside transaction block and the locks
> keep being held unnecessarily. Isn't this problematic? Can we change
> the order of them?

I think there might be problems with that, like perhaps the ErrorData
object can have pointers into the memory contexts that we'd be
destroying in AbortCurrentTransaction().

More generally, I think it's hopeless to try to improve the situation
very much by changing the place where secure_write() happens. It
happens in so many different places, and it is clearly not going to be
possible to make it so that in none of those places do we hold any
important server resources. Therefore I think the only solution is to
fix secure_write() itself ... and the trick is what to do there given
that we have to be very careful not to do anything that might try to
write another message while we are already in the middle of writing a
message.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Hi,

On 2021-08-23 10:13:03 -0400, Robert Haas wrote:
> On Mon, Aug 23, 2021 at 4:15 AM 蔡梦娟(玊于) <mengjuan.cmj@alibaba-inc.com> wrote:
> > I want to know why the interrupt is only handled when ProcDiePending is true, I think query which is supposed to be
canceledalso should respond to the signal.
 
> 
> Well, if we're halfway through sending a message to the client and we
> abort the write, we have no way of re-establishing protocol sync,
> right? It's OK to die without sending any more data to the client,
> since then the connection is closed anyway and the loss of sync
> doesn't matter, but continuing the session doesn't seem workable.

Right.


> Your proposed patch actually seems to dodge this problem and I think
> perhaps we could consider something along those lines. It would be
> interesting to hear what Andres thinks about this idea, since I think
> he was the last one to rewrite that code.

I think it's a reasonable idea. I have some quibbles with the implementation
(new code should be in ProcessClientWriteInterrupt(), not secure_write()), and
I suspect we should escalate more quickly to killing the connection, but those
seem like details.

I think that if we want to tackle this, we need to do the same for
secure_read() as well. secure_read() will process interrupts normally while
DoingCommandRead, but if we're already in a command, we'd just as well be
prevented from processing a !ProcDiePending interrupt.

Greetings,

Andres Freund

Hi,

On 2021-08-27 08:27:38 -0400, Robert Haas wrote:
> On Tue, Aug 24, 2021 at 9:58 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> > to report an error to a client, and then calls AbortCurrentTransaction()
> > to abort the transaction. If secure_write() called by EmitErrorReport()
> > gets stuck, a backend gets stuck inside transaction block and the locks
> > keep being held unnecessarily. Isn't this problematic? Can we change
> > the order of them?
> ...
> More generally, I think it's hopeless to try to improve the situation
> very much by changing the place where secure_write() happens. It
> happens in so many different places, and it is clearly not going to be
> possible to make it so that in none of those places do we hold any
> important server resources. Therefore I think the only solution is to
> fix secure_write() itself ... and the trick is what to do there given
> that we have to be very careful not to do anything that might try to
> write another message while we are already in the middle of writing a
> message.

I wonder if we could improve the situation somewhat by using the non-blocking
pqcomm functions in a few select places. E.g. if elog.c's
send_message_to_frontend() sent its message via a new pq_endmessage_noblock()
(which'd use the existing pq_putmessage_noblock()) and used
pq_flush_if_writable() instead of pq_flush(), we'd a) not block sending to the
client before AbortCurrentTransaction(), b) able to queue further error
messages safely.

I think this'd not violate the goal of putting pq_flush() into
send_message_to_frontend():
    /*
     * This flush is normally not necessary, since postgres.c will flush out
     * waiting data when control returns to the main loop. But it seems best
     * to leave it here, so that the client has some clue what happened if the
     * backend dies before getting back to the main loop ... error/notice
     * messages should not be a performance-critical path anyway, so an extra
     * flush won't hurt much ...
     */
    pq_flush();

because the only situations where we'd not send the data out immediately would
be when the socket buffer is already full. In which case the client wouldn't
get the error immediately anyway?

Greetings,

Andres Freund

On Fri, Aug 27, 2021 at 3:24 PM Andres Freund <andres@anarazel.de> wrote:
> I wonder if we could improve the situation somewhat by using the non-blocking
> pqcomm functions in a few select places. E.g. if elog.c's
> send_message_to_frontend() sent its message via a new pq_endmessage_noblock()
> (which'd use the existing pq_putmessage_noblock()) and used
> pq_flush_if_writable() instead of pq_flush(), we'd a) not block sending to the
> client before AbortCurrentTransaction(), b) able to queue further error
> messages safely.

pq_flush_if_writable() could succeed in sending only part of the data,
so I don't see how this works.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Hi,

On Fri, Aug 27, 2021, at 13:07, Robert Haas wrote:
> On Fri, Aug 27, 2021 at 3:24 PM Andres Freund <andres@anarazel.de> wrote:
> > I wonder if we could improve the situation somewhat by using the non-blocking
> > pqcomm functions in a few select places. E.g. if elog.c's
> > send_message_to_frontend() sent its message via a new pq_endmessage_noblock()
> > (which'd use the existing pq_putmessage_noblock()) and used
> > pq_flush_if_writable() instead of pq_flush(), we'd a) not block sending to the
> > client before AbortCurrentTransaction(), b) able to queue further error
> > messages safely.
> 
> pq_flush_if_writable() could succeed in sending only part of the data,
> so I don't see how this works.

All the data is buffered though, so I don't see that problem that causes?

Andres

On Fri, Aug 27, 2021 at 4:10 PM Andres Freund <andres@anarazel.de> wrote:
> On Fri, Aug 27, 2021, at 13:07, Robert Haas wrote:
> > On Fri, Aug 27, 2021 at 3:24 PM Andres Freund <andres@anarazel.de> wrote:
> > > I wonder if we could improve the situation somewhat by using the non-blocking
> > > pqcomm functions in a few select places. E.g. if elog.c's
> > > send_message_to_frontend() sent its message via a new pq_endmessage_noblock()
> > > (which'd use the existing pq_putmessage_noblock()) and used
> > > pq_flush_if_writable() instead of pq_flush(), we'd a) not block sending to the
> > > client before AbortCurrentTransaction(), b) able to queue further error
> > > messages safely.
> >
> > pq_flush_if_writable() could succeed in sending only part of the data,
> > so I don't see how this works.
>
> All the data is buffered though, so I don't see that problem that causes?

OK, I guess I'm confused here.

If we're always buffering the data, then I suppose there's no risk of
injecting a protocol message into the middle of some other protocol
message, assuming that we don't have a non-local transfer of control
halfway through putting a message in the buffer. But there's still the
risk of getting out of step with the client. Suppose the client does
SELECT 1/0 and we send an ErrorResponse complaining about the division
by zero. But as we're trying to send that response, we block. Later, a
query cancel occurs. We can't queue another ErrorResponse because the
client will interpret that as the response to the next query, since
the division by zero error is the response to the current one. I don't
think that changing pq_flush() to pq_flush_if_writable() in elog.c or
anywhere else can fix that problem.

But that doesn't mean that it isn't a good idea. Any place where we're
doing a pq_flush() and know that another one will happen soon
afterward, before we wait for data from the client, can be changed to
pq_flush_if_writable() without harm, and it's beneficial to do so,
because like you say, it avoids blocking in places that users may find
inconvenient - e.g. while holding locks, as Fujii-san said. The
comment here claims that "postgres.c will flush out waiting data when
control returns to the main loop" but the only pq_flush() call that's
directly present in postgres.c is in response to receiving a Flush
message, so I suppose this is actually talking about the pq_flush()
inside ReadyForQuery. It's not 100% clear to me that we do that in all
relevant cases, though. Suppose we hit an error while processing some
protocol message that does not set send_ready_for_query = true, like
for example Describe ('D'). I think in that case the flush in elog.c
is the only one. Perhaps we ought to change postgres.c so that if we
don't enter the block guarded by "if (send_ready_for_query)" we
instead pq_flush().

-- 
Robert Haas
EDB: http://www.enterprisedb.com

On 2021/09/22 1:14, 蔡梦娟(玊于) wrote:
> Hi all, I want to know your opinion on this patch, or in what way do you think we should solve this problem?

I agree that something like the patch (i.e., introduction of promotion
from cancel request to terminate one) is necessary for the fix. One concern
on the patch is that the cancel request can be promoted to the terminate one
even when secure_write() doesn't actually get stuck. Is this acceptable?
Maybe I'm tempted to set up the duration until the promotion happens....
Or we should introduce the dedicated timer for communication on the socket?

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION