Fix potential hang in right join #6690

windtalker · 2023-01-30T03:25:36Z

What problem does this PR solve?

Issue Number: ref #6528 #6692

Problem Summary:
#6576 introduce a potential bug that for a right join, if probe thread meet error, some probe thread maybe hang forever.

What is changed and how it works?

fix potential hang bug in right join
refine the sync logical in join for both build and probe.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

None

ti-chi-bot · 2023-01-30T03:25:38Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

SeaRise
yibin87

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

SeaRise · 2023-01-30T03:51:29Z

dbms/src/DataStreams/CreatingSetsBlockInputStream.cpp

@@ -120,7 +120,7 @@ void CreatingSetsBlockInputStream::createAll()
            for (auto & elem : subqueries_for_sets)
            {
                if (elem.second.join)
-                    elem.second.join->setBuildTableState(Join::BuildTableState::WAITING);
+                    elem.second.join->setInitActiveBuildConcurrency();


how about moving to PhysicalJoin?

Currently, it must be set after compile, see the comments in https://github.com/pingcap/tiflash/pull/6690/files#diff-e6f85ec76cd474ca862de277609c1dfbc11aa70692a3acf3f4e85fefb649cb65R473-R474

SeaRise

lgtm

gengliqi · 2023-01-30T05:59:38Z

dbms/src/Interpreters/Join.cpp

+void Join::waitUntilAllProbeFinished() const
+{
+    std::unique_lock lock(build_probe_mutex);
+    probe_cv.wait(lock, [&]() {
+        return meet_error || active_probe_concurrency == 0;
+    });
+    if (meet_error)
+        throw Exception("Join meet error before all probe finished!");
+}
+
+void Join::waitUntilAllBuildFinished() const
+{
+    std::unique_lock lk(build_probe_mutex);
+
+    build_cv.wait(lk, [&]() { return meet_error || active_build_concurrency == 0; });
+    if (meet_error) /// throw this exception once failed to build the hash table
+        throw Exception("Build failed before join probe!");
+}


These two similar functions have different code styles. (e.g. indentation and blank lines)

gengliqi · 2023-01-30T06:13:30Z

dbms/src/DataStreams/CreatingSetsBlockInputStream.cpp

-
-        if (subquery.join)
-        {
-            FAIL_POINT_TRIGGER_EXCEPTION(FailPoints::exception_mpp_hash_build);


extern const char exception_mpp_hash_build[]; in this file can also be deleted.

yibin87

LGTM

gengliqi

LGTM

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

windtalker · 2023-01-30T07:39:24Z

/merge

ti-chi-bot · 2023-01-30T07:39:25Z

@windtalker: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

ti-chi-bot · 2023-01-30T07:39:27Z

This pull request has been accepted and is ready to merge.

Commit hash: 4f1218c

SeaRise · 2023-01-30T08:07:42Z

/run-unit-test

SeaRise · 2023-01-30T09:00:49Z

/run-integration-test

ti-chi-bot · 2023-01-30T09:14:04Z

@windtalker: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

trigger some heavy tests which will not run always when PR updated.

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

windtalker · 2023-01-30T09:58:28Z

/run-integration-test

windtalker · 2023-01-30T10:16:20Z

/run-integration-test

SeaRise · 2023-01-30T11:19:24Z

/run-integration-test

windtalker · 2023-01-30T12:11:54Z

/run-integration-test

ref pingcap#6528, ref pingcap#6692 Signed-off-by: ywqzzy <592838129@qq.com>

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 30, 2023

SeaRise self-requested a review January 30, 2023 03:27

SeaRise reviewed Jan 30, 2023

View reviewed changes

windtalker force-pushed the fix_potential_hang_in_right_join branch from b8cd858 to f22eceb Compare January 30, 2023 04:23

SeaRise approved these changes Jan 30, 2023

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Jan 30, 2023

gengliqi reviewed Jan 30, 2023

View reviewed changes

windtalker force-pushed the fix_potential_hang_in_right_join branch from 32b5c92 to 15e0ef9 Compare January 30, 2023 06:40

yibin87 approved these changes Jan 30, 2023

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jan 30, 2023

gengliqi approved these changes Jan 30, 2023

View reviewed changes

windtalker added 6 commits January 30, 2023 15:38

fix potential hang in right join

b11f074

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

add some comments

eae15f2

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

update comments

a47cb07

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

add comments

2618b0a

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

fix typo

703654d

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

address comments

4f1218c

Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>

windtalker force-pushed the fix_potential_hang_in_right_join branch from 15e0ef9 to 4f1218c Compare January 30, 2023 07:39

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jan 30, 2023

Merge branch 'master' into fix_potential_hang_in_right_join

de04338

Merge branch 'master' into fix_potential_hang_in_right_join

af7edc8

ti-chi-bot merged commit bca56b9 into pingcap:master Jan 30, 2023

windtalker deleted the fix_potential_hang_in_right_join branch January 30, 2023 12:46

ywqzzy pushed a commit to ywqzzy/tiflash_1 that referenced this pull request Feb 13, 2023

Fix potential hang in right join (pingcap#6690)

4ab534a

ref pingcap#6528, ref pingcap#6692 Signed-off-by: ywqzzy <592838129@qq.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix potential hang in right join #6690

Fix potential hang in right join #6690

windtalker commented Jan 30, 2023 •

edited by yibin87

Loading

ti-chi-bot commented Jan 30, 2023 •

edited

Loading

SeaRise Jan 30, 2023 •

edited

Loading

windtalker Jan 30, 2023

SeaRise left a comment

gengliqi Jan 30, 2023

windtalker Jan 30, 2023

gengliqi Jan 30, 2023

windtalker Jan 30, 2023

yibin87 left a comment

gengliqi left a comment

windtalker commented Jan 30, 2023

ti-chi-bot commented Jan 30, 2023

ti-chi-bot commented Jan 30, 2023

SeaRise commented Jan 30, 2023

SeaRise commented Jan 30, 2023

ti-chi-bot commented Jan 30, 2023

windtalker commented Jan 30, 2023

windtalker commented Jan 30, 2023

SeaRise commented Jan 30, 2023

windtalker commented Jan 30, 2023

Fix potential hang in right join #6690

Fix potential hang in right join #6690

Conversation

windtalker commented Jan 30, 2023 • edited by yibin87 Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Jan 30, 2023 • edited Loading

SeaRise Jan 30, 2023 • edited Loading

Choose a reason for hiding this comment

windtalker Jan 30, 2023

Choose a reason for hiding this comment

SeaRise left a comment

Choose a reason for hiding this comment

gengliqi Jan 30, 2023

Choose a reason for hiding this comment

windtalker Jan 30, 2023

Choose a reason for hiding this comment

gengliqi Jan 30, 2023

Choose a reason for hiding this comment

windtalker Jan 30, 2023

Choose a reason for hiding this comment

yibin87 left a comment

Choose a reason for hiding this comment

gengliqi left a comment

Choose a reason for hiding this comment

windtalker commented Jan 30, 2023

ti-chi-bot commented Jan 30, 2023

ti-chi-bot commented Jan 30, 2023

SeaRise commented Jan 30, 2023

SeaRise commented Jan 30, 2023

ti-chi-bot commented Jan 30, 2023

windtalker commented Jan 30, 2023

windtalker commented Jan 30, 2023

SeaRise commented Jan 30, 2023

windtalker commented Jan 30, 2023

windtalker commented Jan 30, 2023 •

edited by yibin87

Loading

ti-chi-bot commented Jan 30, 2023 •

edited

Loading

SeaRise Jan 30, 2023 •

edited

Loading