Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix data-race DatabaseReplicated::startupTables()/canExecuteReplicatedMetadataAlter() #52490

Merged
merged 2 commits into from
Jul 25, 2023
Merged

Fix data-race DatabaseReplicated::startupTables()/canExecuteReplicatedMetadataAlter() #52490

merged 2 commits into from
Jul 25, 2023

Conversation

azat
Copy link
Member

@azat azat commented Jul 23, 2023 •
edited
Loading

CI founds 1, and looks like it is safe to just the re-order the code sections since in startupTables() we already have all tables:

Exception: Sanitizer assert found for instance ==================
WARNING: ThreadSanitizer: data race (pid=348)
  Write of size 8 at 0x7b58000044a0 by main thread:
    2 DB::DatabaseReplicated::startupTables(ThreadPoolImpl<ThreadFromGlobalPoolImpl<false>>&, DB::LoadingStrictnessLevel) build_docker/./src/Databases/DatabaseReplicated.cpp:526:16 (clickhouse+0x1ec45092)
    3 DB::TablesLoader::startupTables() build_docker/./src/Databases/TablesLoader.cpp:87:26 (clickhouse+0x1f9258ab) (BuildId: 7d4ce55d33d4c3e3df9fd39b304e67e53eb61a63)
    4 DB::loadMetadata(std::__1::shared_ptr<DB::Context>, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&) build_docker/./src/Interpreters/loadMetadata.cpp:234:12 (clickhouse+0x1fff3834) (BuildId: 7d4ce55d33d4c3e3df9fd39b304e67e53eb61a63)
    5 DB::Server::main() build_docker/./programs/server/Server.cpp:1615:9 (clickhouse+0x163e7f78) (BuildId: 7d4ce55d33d4c3e3df9fd39b304e67e53eb61a63)
    6 Poco::Util::Application::run() build_docker/./base/poco/Util/src/Application.cpp:315:8 (clickhouse+0x257608fe) (BuildId: 7d4ce55d33d4c3e3df9fd39b304e67e53eb61a63)
    7 DB::Server::run() build_docker/./programs/server/Server.cpp:391:25 (clickhouse+0x163d7d7c) (BuildId: 7d4ce55d33d4c3e3df9fd39b304e67e53eb61a63)
    8 Poco::Util::ServerApplication::run(int, char**) build_docker/./base/poco/Util/src/ServerApplication.cpp:131:9 (clickhouse+0x25780114) (BuildId: 7d4ce55d33d4c3e3df9fd39b304e67e53eb61a63)
    9 mainEntryClickHouseServer(int, char**) build_docker/./programs/server/Server.cpp:196:20 (clickhouse+0x163d4c23) (BuildId: 7d4ce55d33d4c3e3df9fd39b304e67e53eb61a63)
    10 main build_docker/./programs/main.cpp:487:12 (clickhouse+0xdf8c877) (BuildId: 7d4ce55d33d4c3e3df9fd39b304e67e53eb61a63)

  Previous read of size 8 at 0x7b58000044a0 by thread T27 (mutexes: write M0, write M1):
    1 DB::DatabaseReplicated::canExecuteReplicatedMetadataAlter() const build_docker/./src/Databases/DatabaseReplicated.cpp:1303:12 (clickhouse+0x1ec5c5bd)
    2 DB::ReplicatedMergeTreeQueue::shouldExecuteLogEntry() const build_docker/./src/Storages/MergeTree/ReplicatedMergeTreeQueue.cpp:1471:24 (clickhouse+0x2115fb56) (BuildId: 7d4ce55d33d4c3e3df9fd39b304e67e53eb61a63)
    3 DB::ReplicatedMergeTreeQueue::selectEntryToProcess(DB::MergeTreeDataMergerMutator&, DB::MergeTreeData&) build_docker/./src/Storages/MergeTree/ReplicatedMergeTreeQueue.cpp:1676:13 (clickhouse+0x21163c58) (BuildId: 7d4ce55d33d4c3e3df9fd39b304e67e53eb61a63)
    4 DB::StorageReplicatedMergeTree::selectQueueEntry() build_docker/./src/Storages/StorageReplicatedMergeTree.cpp:3240:26 (clickhouse+0x20823db2) (BuildId: 7d4ce55d33d4c3e3df9fd39b304e67e53eb61a63)
    5 DB::StorageReplicatedMergeTree::scheduleDataProcessingJob(DB::BackgroundJobsAssignee&) build_docker/./src/Storages/StorageReplicatedMergeTree.cpp:3304:65 (clickhouse+0x208240fc) (BuildId: 7d4ce55d33d4c3e3df9fd39b304e67e53eb61a63)

Changelog category (leave one):

  • Bug Fix (user-visible misbehavior in an official stable release)

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

Fix data-race DatabaseReplicated::startupTables()/canExecuteReplicatedMetadataAlter()

Fixes: #51501 (cc @tavplubix )
Fixes: #52529

@robot-clickhouse-ci-1 robot-clickhouse-ci-1 added the pr-bugfix Pull request with bugfix, not backported by default label Jul 23, 2023
@robot-clickhouse-ci-1
Copy link
Contributor

robot-clickhouse-ci-1 commented Jul 23, 2023 •
edited by robot-clickhouse
Loading

This is an automated comment for commit d1967e1 with description of existing statuses. It's updated for the latest CI running
The full report is available here
The overall status of the commit is 🟡 pending

Check nameDescriptionStatus
CI runningA meta-check that indicates the running CI. Normally, it's in success or pending state. The failed status indicates some problems with the PR🟡 pending
Mergeable CheckChecks if all other necessary checks are successful🟢 success
Push to DockerhubThe check for building and pushing the CI related docker images to docker hub🟢 success

@azat azat mentioned this pull request Jul 23, 2023
Merged
@azat azat marked this pull request as draft July 23, 2023 20:37
@azat azat mentioned this pull request Jul 23, 2023
Merged
@azat azat force-pushed the db/replicated-fix-race branch from f10a739 to ed1ecf2 Compare July 24, 2023 04:12
@azat azat marked this pull request as ready for review July 24, 2023 04:25
@tavplubix tavplubix self-assigned this Jul 24, 2023
@tavplubix tavplubix mentioned this pull request Jul 24, 2023
Closed
@azat azat force-pushed the db/replicated-fix-race branch from ed1ecf2 to 75b1fdf Compare July 24, 2023 14:24
@azat
Fix data-race DatabaseReplicated::startupTables()/canExecuteReplicate…
9e2526a 
…dMetadataAlter()

CI founds [1]:

    Exception: Sanitizer assert found for instance ==================
    WARNING: ThreadSanitizer: data race (pid=348)
      Write of size 8 at 0x7b58000044a0 by main thread:
        2 DB::DatabaseReplicated::startupTables(ThreadPoolImpl<ThreadFromGlobalPoolImpl<false>>&, DB::LoadingStrictnessLevel) build_docker/./src/Databases/DatabaseReplicated.cpp:526:16 (clickhouse+0x1ec45092)
        3 DB::TablesLoader::startupTables() build_docker/./src/Databases/TablesLoader.cpp:87:26 (clickhouse+0x1f9258ab) (BuildId: 7d4ce55d33d4c3e3df9fd39b304e67e53eb61a63)
        4 DB::loadMetadata(std::__1::shared_ptr<DB::Context>, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&) build_docker/./src/Interpreters/loadMetadata.cpp:234:12 (clickhouse+0x1fff3834) (BuildId: 7d4ce55d33d4c3e3df9fd39b304e67e53eb61a63)
        5 DB::Server::main() build_docker/./programs/server/Server.cpp:1615:9 (clickhouse+0x163e7f78) (BuildId: 7d4ce55d33d4c3e3df9fd39b304e67e53eb61a63)
        6 Poco::Util::Application::run() build_docker/./base/poco/Util/src/Application.cpp:315:8 (clickhouse+0x257608fe) (BuildId: 7d4ce55d33d4c3e3df9fd39b304e67e53eb61a63)
        7 DB::Server::run() build_docker/./programs/server/Server.cpp:391:25 (clickhouse+0x163d7d7c) (BuildId: 7d4ce55d33d4c3e3df9fd39b304e67e53eb61a63)
        8 Poco::Util::ServerApplication::run(int, char**) build_docker/./base/poco/Util/src/ServerApplication.cpp:131:9 (clickhouse+0x25780114) (BuildId: 7d4ce55d33d4c3e3df9fd39b304e67e53eb61a63)
        9 mainEntryClickHouseServer(int, char**) build_docker/./programs/server/Server.cpp:196:20 (clickhouse+0x163d4c23) (BuildId: 7d4ce55d33d4c3e3df9fd39b304e67e53eb61a63)
        10 main build_docker/./programs/main.cpp:487:12 (clickhouse+0xdf8c877) (BuildId: 7d4ce55d33d4c3e3df9fd39b304e67e53eb61a63)

      Previous read of size 8 at 0x7b58000044a0 by thread T27 (mutexes: write M0, write M1):
        1 DB::DatabaseReplicated::canExecuteReplicatedMetadataAlter() const build_docker/./src/Databases/DatabaseReplicated.cpp:1303:12 (clickhouse+0x1ec5c5bd)
        2 DB::ReplicatedMergeTreeQueue::shouldExecuteLogEntry() const build_docker/./src/Storages/MergeTree/ReplicatedMergeTreeQueue.cpp:1471:24 (clickhouse+0x2115fb56) (BuildId: 7d4ce55d33d4c3e3df9fd39b304e67e53eb61a63)
        3 DB::ReplicatedMergeTreeQueue::selectEntryToProcess(DB::MergeTreeDataMergerMutator&, DB::MergeTreeData&) build_docker/./src/Storages/MergeTree/ReplicatedMergeTreeQueue.cpp:1676:13 (clickhouse+0x21163c58) (BuildId: 7d4ce55d33d4c3e3df9fd39b304e67e53eb61a63)
        4 DB::StorageReplicatedMergeTree::selectQueueEntry() build_docker/./src/Storages/StorageReplicatedMergeTree.cpp:3240:26 (clickhouse+0x20823db2) (BuildId: 7d4ce55d33d4c3e3df9fd39b304e67e53eb61a63)
        5 DB::StorageReplicatedMergeTree::scheduleDataProcessingJob(DB::BackgroundJobsAssignee&) build_docker/./src/Storages/StorageReplicatedMergeTree.cpp:3304:65 (clickhouse+0x208240fc) (BuildId: 7d4ce55d33d4c3e3df9fd39b304e67e53eb61a63)

  [1]: https://s3.amazonaws.com/clickhouse-test-reports/52395/0b258dda4ee618a4d002e2b5246d68bbd2c77c7e/integration_tests__tsan__[5_6].html

Add ddl_worker_initialized flag to avoid this race.

Note, that it should be enough to check this flag only in
canExecuteReplicatedMetadataAlter() since only it can be run in parallel
with ctor before it had been finished.

v0: initialize ddl before startupTables()
v2: ddl_worker_initialized

Signed-off-by: Azat Khuzhin <[email protected]>
@azat azat force-pushed the db/replicated-fix-race branch from 75b1fdf to 9e2526a Compare July 24, 2023 14:27
@alexey-milovidov
Copy link
Member

Some tests started to time out.

@azat
Copy link
Member Author

azat commented Jul 25, 2023 •
edited
Loading

This are unlikely related, because all of this 4 tests failed on other PRs as well - https://play.clickhouse.com/play?user=play#U0VMRUNUIHRlc3RfbmFtZSwgY291bnRJZih0ZXN0X2R1cmF0aW9uX21zID4gNjAwZTMpIHRpbWVvdXRzLCBjb3VudCgpIHJ1bnMsIGdyb3VwVW5pcUFycmF5KGNoZWNrX25hbWUpCkZST00gY2hlY2tzCldIRVJFIHRlc3RfbmFtZSBpbiAoJzAwNzE5X3BhcmFsbGVsX2RkbF90YWJsZScsICcwMTkyM19uZXR3b3JrX3JlY2VpdmVfdGltZV9tZXRyaWNfaW5zZXJ0JywgJzAxMDM1X2F2Z193ZWlnaHRlZF9sb25nJywgJzAxODIxX2pvaW5fdGFibGVfcmFjZV9sb25nJykKICAgIEFORCBjaGVja19zdGFydF90aW1lID49IG5vdygpIC0gSU5URVJWQUwgMjQgZGF5CiAgICBBTkQgdGVzdF9zdGF0dXMgIT0gJ1NLSVBQRUQnCiAgICBBTkQgdGVzdF9zdGF0dXMgTElLRSAnRiUnCiAgICBBTkQgY2hlY2tfc3RhdHVzICE9ICdzdWNjZXNzJwpHUk9VUCBCWSAxCk9SREVSIEJZIDE=

OK, here are the fixes for all of this 4 tests:

@tavplubix tavplubix merged commit 85082ad into ClickHouse:master Jul 25, 2023
@azat azat deleted the db/replicated-fix-race branch July 25, 2023 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tavplubix tavplubix tavplubix approved these changes

Assignees

@tavplubix tavplubix

Labels
pr-bugfix Pull request with bugfix, not backported by default
Projects
None yet
Milestone
No milestone
4 participants
@azat @robot-clickhouse-ci-1 @alexey-milovidov @tavplubix