Avoid deadlocks on JOIN Engine tables

[Algunenano]

Changelog category (leave one):

• Bug Fix (user-visible misbehaviour in official stable or prestable release)

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Avoid deadlocks when reading and writting on JOIN Engine tables at the same time

Detailed description / Documentation draft:

The patch ended up requiring way more changes that I wanted to. I decided to replace the shared_mutex by a RWLock for several reasons:

• The main one because it allows getting a READ lock after you've gotten one already even if there has been a request by other request/thread to get a write lock in the meantime. This was what caused the deadlock at Deadlock with JOIN Engine #29485
• RWLock has profile events, so it's easier to track
• RWLock has timeouts, which is a nice way to avoid deadlocks requiring a database restart (as you can't kill a query that's forever waiting for a mutex).

Since RWLock requires a query_id to work effectively I had to add a context parameter to multiple places. The 2 functions that I didn't add it and instead used RWLockImpl::NO_QUERY were totalRows and totalBytes as their declaration is used in 10 other places, but it could be done if we wanted to.

The added test fails all the time in my system pre-change (usually even in the second iteration of the loop) but I had to increase the size the table until if was reliable deadlocking the database as it's purely timebased.

Fixes #29485
The bug affects all stable releases so it would be great to have it backported to 21.8+ at least. Let me know if I can help there if the proposed solution is accepted.

[vdimir]

Test 01732_race_condition_storage_join_long.sh is interesting with this changes. Now all queries in this test should success.
Would some of them fail with timeout error after change?

[vdimir]

It's not changed in this PR, but I wonder why it's LOGICAL_ERROR, can it be reached in we perform INSERT into JOIN Engine table during SELECT?

UPD: because lock already acquired in StorageJoin::insertBlock, but storage_join_lock can be set only from StorageJoin::getJoinLocked that are used for SELECTing data.

[Algunenano]

Test 01732_race_condition_storage_join_long.sh is interesting with this changes. Now all queries in this test should success.
Would some of them fail with timeout error after change?

Before the change (master) the queries would get stuck forever if a deadlock happens (read starts and gets lock, writes starts and asks for write lock, first read tries to get another read lock). The lock_acquire_timeout setting doesn't have any effect in master because it uses a shared mutex that doesn't respect the timeout.

With this change, the issue should not appear anymore as RWLock allows you to jump the lock queue is you already hold a read lock and request another. This is what fixes the bug. The timeouts only happen in the tests because I set it too low for the sanitizers but if they were large enough it should eventually go through. I'm not testing that the timeout occurs for low enough values of lock_acquire_timeout because there isn't a reliable way that I know to trigger it.

[Algunenano]

Ups I see that you were talking about a different test and not the one added in the PR.

Would some of them fail with timeout error after change?

Not unless the server is extremely slow for other reasons and I would say that would be a desirable outcome (respect the settings instead of blocking forever). In the test itself there are only 4 concurrent queries (3 reads and an insert) so I think it's highly unlikely that they would need to wait for 120 to acquire the lock.

[vdimir]

Not unless the server is extremely slow for other reasons and I would say that would be a desirable outcome (respect the settings instead of blocking forever). In the test itself there are only 4 concurrent queries (3 reads and an insert) so I think it's highly unlikely that they would need to wait for 120 to acquire the lock.

If I'm not mistaken lock is acquired for the whole SELECT query, we get StorageJoin in HashJoin that holds lock during execution. For INSERT lock is acquired per each block.

Another question does RWLock used only because it supports timeouts? Can problem be solved with std::timed_mutex, for instance? As #29485 (comment) says RWLock is a bit specific.

Finally, have you research how difficult to solve problem without timeout? Like to lock mutex in correct order, change logic a bit? What is the main issue here?

[Algunenano]

If I'm not mistaken lock is acquired for the whole SELECT query, we get StorageJoin in HashJoin that holds lock during execution. For INSERT lock is acquired per each block.

For SELECT queries the lock is acquired multiple times, the first one I think happens during InterpreterSelectQuery, and then when read() is called. Might be more.

Another question does RWLock used only because it supports timeouts? Can problem be solved with std::timed_mutex, for instance? As #29485 (comment) says RWLock is a bit specific.

No, the main reason was to avoid the deadlock due to acquiring the same lock multiple times (and somebody else gets in between). shared_timed_mutex would not fix that. Anything else that allows something like this would work in this situation.

Finally, have you research how difficult to solve problem without timeout? Like to lock mutex in correct order, change logic a bit? What is the main issue here?

I think it's doable, ideally by just holding the mutex at the start (InterpreterSelectQuery) and never acquire it again under any circumstance. 2 super tricky situations for show how hard that would be:

• If you use joinGet or JOIN multiple times in different subqueries you would need to make sure you only try to hold the mutex once, and not call it again as otherwise there can be a race condition with a write query between them that can't be fixed by std mutexes (that I know of).
• If you use joinGet over a table and, at the same time, read the size of that table via system.tables you would also try to hold the mutex twice, one at the start and one at runtime as far as I can see. Not sure what would happen if you do several queries concurrently doing an INSERT to the join table from that.

So, from what's available I couldn't see a better option; but I don't say there isn't.

[Algunenano]

I can't access the logs of the failed tests as they point to an internal URL. Are they related to changes in this PR?

[vdimir]

I can't access the logs of the failed tests as they point to an internal URL. Are they related to changes in this PR?

Seems that it isn't task just timed out (I see similar issue in other PRs).

No, the main reason was to avoid the deadlock due to acquiring the same lock multiple times (and somebody else gets in between). shared_timed_mutex would not fix that. Anything else that allows something like this would work in this situation.

What's about std::recursive_timed_mutex ?

[Algunenano]

Seems that it isn't task just timed out (I see similar issue in other PRs).

Thanks!

What's about std::recursive_timed_mutex ?

It only provides exclusive ownership only so you wouldn't be able to have multiple read queries running at the same time.

[vdimir]

Functional stateless tests flaky check (address)
02033_join_engine_deadlock
Test runs too long (> 60s). Make it faster.

Let's mark it as long

[Algunenano]

Manually backported PRs:

• 21.10: Backport #29544 to 21.10: Avoid deadlocks on JOIN Engine tables #30182
• 21.9: (Manual) Backport #29544 to 21.9: Avoid deadlocks on JOIN Engine tables #30185
• 21.8: (Manual) Backport #29544 to 21.8: Avoid deadlocks on JOIN Engine tables #30187

Further backports are possible but they require extra manual work.