Fix race in storage join by vdimir · Pull Request #21009 · ClickHouse/ClickHouse

vdimir · 2021-02-20T15:02:23Z

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

Changelog category (leave one):

Bug Fix

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Block parallel insertions into storage join

Fix race introduced in PR #20079

vdimir · 2021-02-24T16:05:39Z

test_storage_kafka/test.py::test_kafka_no_holes_when_write_suffix_failed not related, failed in master during past three days

…e_long

KochetovNicolai · 2021-02-24T17:41:38Z

Some considerations:

std::shared_lock<std::shared_mutex> storage_join_lock is not needed in HashJoin. HashJoin should not be responsible for external synchronization.
JoinSource is an internal class for HashJoin. It need to get HashJoinPtr + std::shared_lock<std::shared_mutex> storage_join_lock in constructor (btw, why this lock was removed?)
Function JoinGet does not need to have access to HashJoinPtr. We can add StorageJoin::joinGet, which takes std::shared_lock<std::shared_mutex> storage_join_lock and calls HashJoin::joinGet. Lock will be taken for each block, but it is ok (and we will have less chances to have a deadlock).

Also, IMHO, joinGet function should be a member of HashJoin::RightTableData (but this is not important for this pr).

KochetovNicolai · 2021-02-24T17:55:03Z

src/Storages/StorageJoin.h

    /// Access the innards.
-    HashJoinPtr & getJoin() { return join; }
+    std::shared_ptr<HashJoinHolder> getJoin() { return std::make_shared<HashJoinHolder>(rwlock, join); }
    HashJoinPtr getJoin(std::shared_ptr<TableJoin> analyzed_join) const;


I think this method should actually return HashJoinHolder (looks a little bit more logical then having shared_lock inside HashJoin) .

We discussed that it will requires changes in ExpressionAnalyzer, it will have to handle this lock. So decided to keep lock in HashJoin.

Move joinGet into StorageJoin Protect JoinSource with lock, add test Add comments about locking logic

vdimir · 2021-02-25T09:38:42Z

@KochetovNicolai

std::shared_lock<std::shared_mutex> storage_join_lock is not needed in HashJoin. HashJoin should not be responsible for external synchronization.

We discussed that it will requires changes in ExpressionAnalyzer, it will have to handle this lock. So decided to keep lock in HashJoin and add mote detailed comments to code.

JoinSource is an internal class for HashJoin. It need to get HashJoinPtr + std::shared_lock<std::shared_mutex> storage_join_lock in constructor (btw, why this lock was removed?)

Yes, lock removed accidentally, I fixed it and added into test.

Function JoinGet does not need to have access to HashJoinPtr. We can add StorageJoin::joinGet, which takes std::shared_lock<std::shared_mutex> storage_join_lock and calls HashJoin::joinGet. Lock will be taken for each block, but it is ok (and we will have less chances to have a deadlock).

I moved joinGet into StorageJoin, and take lock for each block, because used_flags are not required in this method.

…g logic

KochetovNicolai · 2021-02-25T10:28:13Z

src/Functions/FunctionJoinGet.h

 public:
-    ExecutableFunctionJoinGet(HashJoinPtr join_, const DB::Block & result_columns_)
-        : join(std::move(join_)), result_columns(result_columns_) {}
+    ExecutableFunctionJoinGet(StorageJoinPtr storage_join_, const DB::Block & result_columns_)


I think we also need to hold TableLockHolder here along with storage.

KochetovNicolai · 2021-02-25T10:30:17Z

src/Interpreters/HashJoin.cpp


    {
-        std::unique_lock lock(data->rwlock);
+        assert(storage_join_lock.mutex() == nullptr);


Hm, I think exception will be better here, cause it is not a class invariant.

KochetovNicolai · 2021-02-25T10:34:56Z

src/Storages/StorageJoin.h

+    DataTypePtr joinGetCheckAndGetReturnType(const DataTypes & data_types, const String & column_name, bool or_null) const;
+    ColumnWithTypeAndName joinGet(const Block & block, const Block & block_with_columns_to_add) const;


Need some comments for this methods.

…unctionJoinGet

…join

Backport #21009 to 21.2: Fix race in storage join

Backport #21009 to 21.1: Fix race in storage join

Try to fix race in storage join: block parralel inserts

6cc2fb5

vdimir added the pr-not-for-changelog This PR should not be mentioned in the changelog label Feb 20, 2021

robot-clickhouse added pr-bugfix Pull request with bugfix, not backported by default and removed pr-not-for-changelog This PR should not be mentioned in the changelog labels Feb 20, 2021

vdimir changed the title ~~Try to fix race in storage join: block parallel inserts~~ Fix race in storage join Feb 20, 2021

vdimir marked this pull request as ready for review February 20, 2021 18:02

alexey-milovidov mentioned this pull request Feb 21, 2021

21.3 release checklist #20182

Closed

vdimir added 2 commits February 23, 2021 19:27

Add test race_condition_storage.sh

e4fdbbf

Lock before reuseJoinedData

c006b41

vdimir added pr-not-for-changelog This PR should not be mentioned in the changelog and removed pr-not-for-changelog This PR should not be mentioned in the changelog labels Feb 24, 2021

KochetovNicolai self-assigned this Feb 24, 2021

vdimir added 2 commits February 24, 2021 19:19

Remove unused *Locked methods from HashJoin, add comments

6aa4791

Rename 01731_race_condition_storage.sh -> 01732_race_condition_storag…

f93e5b8

…e_long

KochetovNicolai reviewed Feb 24, 2021

View reviewed changes

Update StorageJoin locking

2f70e89

Move joinGet into StorageJoin Protect JoinSource with lock, add test Add comments about locking logic

Add static_assert to joinGet to keep information about current lockin…

f1c6010

…g logic

KochetovNicolai reviewed Feb 25, 2021

View reviewed changes

KochetovNicolai approved these changes Feb 25, 2021

View reviewed changes

vdimir added 2 commits February 25, 2021 14:21

Minor changes in Hash/StorageJoin, add TableLockHolder to ExecutableF…

6f864dd

…unctionJoinGet

Merge remote-tracking branch 'upstream/master' into fix-race-storage-…

9c1ba71

…join

vdimir merged commit 78024ae into ClickHouse:master Feb 26, 2021

vdimir deleted the fix-race-storage-join branch February 26, 2021 11:26

This was referenced Feb 26, 2021

Cherry pick #21009 to 21.1: Fix race in storage join #21240

Merged

Cherry pick #21009 to 21.2: Fix race in storage join #21241

Merged

This was referenced Feb 26, 2021

Cherry pick #21009 to 20.12: Fix race in storage join #21242

Merged

Cherry pick #21009 to 20.8: Fix race in storage join #21243

Closed

Cherry pick #21009 to 20.11: Fix race in storage join #21244

Closed

robot-clickhouse mentioned this pull request Feb 26, 2021

Backport #21009 to 20.12: Fix race in storage join #21249

Merged

robot-clickhouse pushed a commit that referenced this pull request Feb 26, 2021

Backport #21009 to 20.12: Fix race in storage join

66e4229

robot-clickhouse mentioned this pull request Feb 26, 2021

Backport #21009 to 21.1: Fix race in storage join #21250

Merged

robot-clickhouse pushed a commit that referenced this pull request Feb 26, 2021

Backport #21009 to 21.1: Fix race in storage join

e6ad745

robot-clickhouse pushed a commit that referenced this pull request Feb 26, 2021

Backport #21009 to 21.2: Fix race in storage join

d44c548

robot-clickhouse mentioned this pull request Feb 26, 2021

Backport #21009 to 21.2: Fix race in storage join #21251

Merged

vdimir added a commit that referenced this pull request Feb 28, 2021

Merge pull request #21251 from ClickHouse/backport/21.2/21009

a326138

Backport #21009 to 21.2: Fix race in storage join

vdimir added a commit that referenced this pull request Feb 28, 2021

Merge pull request #21250 from ClickHouse/backport/21.1/21009

390e2b9

Backport #21009 to 21.1: Fix race in storage join

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix race in storage join#21009

Fix race in storage join#21009
vdimir merged 9 commits intoClickHouse:masterfrom
vdimir:fix-race-storage-join

vdimir commented Feb 20, 2021 •

edited

Loading

Uh oh!

vdimir commented Feb 24, 2021

Uh oh!

KochetovNicolai commented Feb 24, 2021

Uh oh!

KochetovNicolai Feb 24, 2021

Uh oh!

vdimir Feb 25, 2021

Uh oh!

vdimir commented Feb 25, 2021 •

edited

Loading

Uh oh!

KochetovNicolai Feb 25, 2021

Uh oh!

KochetovNicolai Feb 25, 2021

Uh oh!

KochetovNicolai Feb 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		DataTypePtr joinGetCheckAndGetReturnType(const DataTypes & data_types, const String & column_name, bool or_null) const;
		ColumnWithTypeAndName joinGet(const Block & block, const Block & block_with_columns_to_add) const;

Conversation

vdimir commented Feb 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vdimir commented Feb 24, 2021

Uh oh!

KochetovNicolai commented Feb 24, 2021

Uh oh!

KochetovNicolai Feb 24, 2021

Choose a reason for hiding this comment

Uh oh!

vdimir Feb 25, 2021

Choose a reason for hiding this comment

Uh oh!

vdimir commented Feb 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KochetovNicolai Feb 25, 2021

Choose a reason for hiding this comment

Uh oh!

KochetovNicolai Feb 25, 2021

Choose a reason for hiding this comment

Uh oh!

KochetovNicolai Feb 25, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vdimir commented Feb 20, 2021 •

edited

Loading

vdimir commented Feb 25, 2021 •

edited

Loading