Re: Walsender timeouts and large transactions

On 2017-08-09 16:23, Petr Jelinek wrote:
> On 02/08/17 19:35, Yura Sokolov wrote:
>> The following review has been posted through the commitfest 
>> application:
>> make installcheck-world:  tested, passed
>> Implements feature:       not tested
>> Spec compliant:           not tested
>> Documentation:            not tested
>> 
>> There is no check for (last_reply_timestamp <= 0 || wal_sender_timeout 
>> <= 0) as in other places
>> (in WalSndKeepaliveIfNecessary for example).
>> 
>> I don't think moving update of 'now' down to end of loop body is 
>> correct:
>> there are calls to ProcessConfigFile with SyncRepInitConfig, 
>> ProcessRepliesIfAny that can
>> last non-negligible time. It could lead to over sleeping due to larger 
>> computed sleeptime.
>> Though I could be mistaken.
>> 
>> I'm not sure about moving `if (!pg_is_send_pending())` in a body loop 
>> after WalSndKeepaliveIfNecessary.
>> Is it necessary? But it looks harmless at least.
>> 
> 
> We also need to do actual timeout handing so that the timeout is not
> deferred to the end of the transaction (Which is why I moved `if
> (!pg_is_send_pending())` under WalSndCheckTimeOut() and
> WalSndKeepaliveIfNecessary() calls).
> 

If standby really stalled, then it will not read from socket, and then
`pg_is_sendpending` eventually will return false, and timeout will be 
checked.
If standby doesn't stall, then `last_reply_timestamp` will be updated in
`ProcessRepliesIfAny`, and so timeout will not be triggered.
Do I understand correctly, or I missed something?

>> Could patch be reduced to check after first `if 
>> (!pg_is_sendpending())` ? like:
>> 
>> 	if (!pq_is_send_pending())
>> -		return;
>> +	{
>> +		if (last_reply_timestamp <= 0 || wal_sender_timeout <= 0)
>> +		{
>> +			CHECK_FOR_INTERRUPTS();
>> +			return;
>> +		}
>> +		if (now <= TimestampTzPlusMilliseconds(last_reply_timestamp, 
>> wal_sender_timeout / 2))
>> +			return;
>> +	}
>> 
>> If not, what problem prevents?
> 
> We should do CHECK_FOR_INTERRUPTS() independently of pq_is_send_pending
> so that it's possible to stop walsender while it's processing large
> transaction.

In this case CHECK_FOR_INTERRUPTS will be called after 
wal_sender_timeout/2.
(This diff is for first appearance of `pq_is_send_pending` in a 
function).

If it should be called more often, then patch could be simplier:

  	if (!pq_is_send_pending())
-		return;
+	{
+		CHECK_FOR_INTERRUPTS();
+		if (last_reply_timestamp <= 0 || wal_sender_timeout <= 0 ||
+				now <= TimestampTzPlusMilliseconds(last_reply_timestamp, 
wal_sender_timeout / 2))
+			return;
+	}

(Still, I could be mistaken, it is just suggestion).

Is it hard to add test for case this patch fixes?

With regards,
-- 
Sokolov Yura aka funny_falcon
Postgres Professional: https://postgrespro.ru
The Russian Postgres Company