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 parse oom #16041

Merged
merged 1 commit into from
May 12, 2024
Merged

fix parse oom #16041

merged 1 commit into from
May 12, 2024

Conversation

daviszhen
Copy link
Contributor

update

What type of PR is this?

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

Which issue(s) this PR fixes:

issue ##15982

#15975

What this PR does / why we need it:

本地sysbench 1000w 没复现出oom。

对2处潜在的stmt引用问题,做了修改。

update
@daviszhen daviszhen changed the title free refs fix parse oom May 12, 2024
@matrix-meow matrix-meow added the size/S Denotes a PR that changes [10,99] lines label May 12, 2024
@mergify mergify bot requested a review from sukki37 May 12, 2024 09:21
@mergify mergify bot added the kind/bug Something isn't working label May 12, 2024
@matrix-meow
Copy link
Contributor

@daviszhen Thanks for your contributions!

Pull Request Review:

Title:

The title "fix parse oom" is concise but could be more descriptive. It would be helpful to have a clearer indication of what specific issue related to parsing out-of-memory (oom) errors is being addressed.

Body:

The body of the pull request is minimal but provides essential information about the type of PR (BUG), the related issues, and a brief note in Chinese mentioning the testing results and modifications made to address potential statement reference issues.

Changes:

  1. In computation_wrapper.go:

    • Added a new method freeStmt() to handle statement freeing.
    • Modified ResetPlanAndStmt() and Free() methods to call freeStmt() for statement cleanup.
    • Moved the cwft.Clear() call to freeStmt() method to avoid redundant clearing.
  2. In mysql_cmd_executor.go:

    • Added statements to set execCtx.stmt, execCtx.cw, and execCtx.cws to nil before freeing resources.

Feedback and Suggestions:

  1. Redundant Clearing in computation_wrapper.go:

    • Issue: The cwft.Clear() method is called both in freeStmt() and Free() methods, leading to redundant clearing of resources.
    • Suggestion: Remove the cwft.Clear() call from the freeStmt() method to avoid unnecessary clearing and ensure clarity in resource management.
  2. Potential Resource Leak in mysql_cmd_executor.go:

    • Issue: Setting execCtx.stmt, execCtx.cw, and execCtx.cws to nil without proper resource cleanup may lead to resource leaks or unexpected behavior.
    • Suggestion: Before setting these variables to nil, ensure that any associated resources are properly released to prevent leaks and maintain proper memory management.
  3. Code Optimization:

    • Consider consolidating resource cleanup logic to a centralized location to avoid duplication and ensure consistent handling of resource cleanup throughout the codebase.
    • Ensure that all resources are properly managed and released to prevent memory leaks and improve the overall stability of the application.
  4. Documentation:

    • It would be beneficial to include comments or documentation explaining the purpose of the changes made in the pull request to help other developers understand the modifications and their impact on the codebase.

By addressing the identified issues and optimizing the code changes, the pull request can enhance the reliability and maintainability of the codebase while ensuring proper resource management and preventing potential memory-related issues.

@mergify mergify bot merged commit 7462a7c into matrixorigin:1.2-dev May 12, 2024
19 checks passed
@aylei aylei mentioned this pull request Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working size/S Denotes a PR that changes [10,99] lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants