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.

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

Fix rightjoin.Free hung #17107

Merged
merged 3 commits into from
Jun 24, 2024
Merged

Fix rightjoin.Free hung #17107

merged 3 commits into from
Jun 24, 2024

Conversation

zengyan1
Copy link
Contributor

@zengyan1 zengyan1 commented Jun 24, 2024

User description

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue https://github.com/matrixorigin/MO-Cloud/issues/3338

What this PR does / why we need it:

Do not read from channel in Free()


PR Type

Bug fix


Description

  • Simplified the condition for sending to the channel in the Free method across multiple files.
  • Removed unnecessary channel reads for merger cases to prevent hanging.

Changes walkthrough 📝

Relevant files
Bug fix
types.go
Simplify channel handling logic in `Free` method.               

pkg/sql/colexec/right/types.go

  • Simplified the condition for sending to the channel in the Free
    method.
  • Removed unnecessary channel reads for merger cases.
  • +2/-8     
    types.go
    Simplify channel handling logic in `Free` method.               

    pkg/sql/colexec/rightanti/types.go

  • Simplified the condition for sending to the channel in the Free
    method.
  • Removed unnecessary channel reads for merger cases.
  • +2/-8     
    types.go
    Simplify channel handling logic in `Free` method.               

    pkg/sql/colexec/rightsemi/types.go

  • Simplified the condition for sending to the channel in the Free
    method.
  • Removed unnecessary channel reads for merger cases.
  • +2/-8     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review [1-5] 2
    🧪 Relevant tests No
    🔒 Security concerns No
    ⚡ Key issues to review Possible Bug:
    The changes in the PR remove the channel read operations for merger cases and modify the condition for sending nil to the channel. Ensure that this change does not introduce any race conditions or deadlocks, particularly in scenarios where arg.NumCPU > 1 and arg.IsMerger is false. It's crucial to verify that the new logic correctly handles all edge cases without causing the system to hang or crash.

    @matrix-meow matrix-meow added the size/S Denotes a PR that changes [10,99] lines label Jun 24, 2024
    @mergify mergify bot requested a review from sukki37 June 24, 2024 07:09
    Copy link

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Close the channel after sending nil to avoid potential deadlocks

    To ensure that the Channel is properly closed and to avoid potential deadlocks, consider
    adding a close(arg.Channel) statement after sending nil to the channel.

    pkg/sql/colexec/right/types.go [132]

     arg.Channel <- nil
    +close(arg.Channel)
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Closing the channel after sending nil can indeed prevent potential deadlocks and ensure proper channel lifecycle management, which is a good practice in Go.

    7
    Enhancement
    Extract complex condition into a well-named boolean variable for readability

    To improve readability and maintainability, consider extracting the condition
    !ctr.handledLast && arg.NumCPU > 1 && !arg.IsMerger into a well-named boolean variable.

    pkg/sql/colexec/right/types.go [131]

    -if !ctr.handledLast && arg.NumCPU > 1 && !arg.IsMerger {
    +shouldSendNil := !ctr.handledLast && arg.NumCPU > 1 && !arg.IsMerger
    +if shouldSendNil {
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Extracting the condition into a boolean variable improves code readability and maintainability, making it easier to understand the logic at a glance.

    6

    @zengyan1
    Copy link
    Contributor Author

    zengyan1 commented Jun 24, 2024

    https://github.com/matrixorigin/mo-nightly-regression/actions/runs/9628576836
    这次测试卡住的原因是在rightjoin.Free中清理channel,当触发提前结束时,若不同并发接受到不同的hashmap,则有可能hung住。
    由于rightjoin.argument有reuse且channel有足够的缓冲,channel是不需要清理的,因此将这一部分删去。

    在rightjoin.build()中,判断arg.Ismerger时在receivehashmap前sleep,否则在receivehashmap后sleep,
    并使用 #17038 添加的bvt case,可以稳定复现。

    @mergify mergify bot merged commit 0eed606 into matrixorigin:1.2-dev Jun 24, 2024
    18 checks passed
    mergify bot pushed a commit that referenced this pull request Jun 27, 2024
    Cherry-pick #17038 and #17107 to main
    
    Approved by: @m-schen
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Bug fix Review effort [1-5]: 2 size/S Denotes a PR that changes [10,99] lines
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    5 participants