Re: Postgres stucks in deadlock detection

On 13.04.2018 18:41, Andres Freund wrote:
> Hi,
>
> On 2018-04-13 16:43:09 +0300, Konstantin Knizhnik wrote:
>> Updated patch is attached.
>> +    /*
>> +     * Ensure that only one backend is checking for deadlock.
>> +     * Otherwise under high load cascade of deadlock timeout expirations can cause stuck of Postgres.
>> +     */
>> +    if (!pg_atomic_test_set_flag(&ProcGlobal->activeDeadlockCheck))
>> +    {
>> +        enable_timeout_after(DEADLOCK_TIMEOUT, DeadlockTimeout);
>> +        return;
>> +    }
>> +    inside_deadlock_check = true;
> I can't see that ever being accepted.  This means there's absolutely no
> bound for deadlock checks happening even under light concurrency, even
> if there's no contention for a large fraction of the time.

It may cause problems only if
1. There is large number of active sessions
2. They perform deadlock-prone queries (so no attempts to avoid 
deadlocks at application level)
3. Deadlock timeout is set to be very small (10 msec?)

Otherwise either probability that all backends  once and once again are 
trying to check deadlocks concurrently is very small (and can be even 
more reduced by using random timeout for subsequent deadlock checks), 
either system can not normally function in any case because large number 
of clients fall into deadlock.


>
> If you want to improve this, improve the efficiency of the
> implementation, check multiple lockers at the same time (set a bit
> afterwards that they don't recheck themselves). There's plenty
> approaches that don't result in a significantly worse user experience.

It seems to me that the best best solution will be to perform deadlock 
by some dedicated process (deadlock checker) with specified frequency,
or do it in backends themselves but ensure that interval between 
deadlock checks exceeds deadlock_timeout.
On both cases it is necessary to check for all deadlock loops, not only 
deadlocks involving self transaction.
And definitely we should be able to abort "foreign" transaction, not 
only transaction performed by this backend.
I do not know how difficult it will be to implement. I expect that it 
will be not so easy, otherwise why deadlock detection was not 
implemented in this way in Postgres from the very beginning...

I completely agree that there are plenty of different approaches, but 
IMHO the currently used strategy is the worst one, because it can stall 
system even if there are not deadlocks at all.
I always think that deadlock is a programmer's error rather than normal 
situation. May be it is wrong assumption but I can not believe that some 
system can normally work when deadlocks happen quite frequently.  
Consider classical example: transfer money from one account to another:

start transaction;
update accounts set balance=balance+? where aid=?;
update accounts set balance=balance-? where aid=?;
commit transactions;

With thousands accounts and random choice of account IDs concurrent 
execution of this transaction by multiple clients (~100) cause deadlocks 
in less than few percents of transactions.
My patch doesn't somehow affect performance and latency in this case.
Obviously that if number of accounts is changed to 10 and number of 
clients to 1000, then deadlocks will happen permanently. But in this 
case system will not be able to work normally either with my patch, 
either without it.

So before implementing some complicated solution of the problem9too slow 
deadlock detection), I think that first it is necessary to understand 
whether there is such problem at al and under which workload it can happen.

-- 
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company