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

Handle system view Processlist Not Available issue #15428

Merged

Conversation

qingxinhome
Copy link
Contributor

What type of PR is this?

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

Which issue(s) this PR fixes:

issue #15423

What this PR does / why we need it:

处理processlist系统视图升级后定义错误问题

@mergify mergify bot added the kind/bug Something isn't working label Apr 10, 2024
@matrix-meow matrix-meow added the size/S Denotes a PR that changes [10,99] lines label Apr 10, 2024
@matrix-meow
Copy link
Contributor

@qingxinhome Thanks for your contributions!

Pull Request Review:

Title:

The title of the pull request is clear and concise, indicating that the purpose of the changes is to handle the issue related to the system view Processlist not being available.

Body:

The body of the pull request provides relevant information about the type of PR (BUG), the specific issue it fixes (issue #15423), and why the PR is needed (to address a definition error after upgrading the processlist system view).

Changes:

  1. pkg/cnservice/upgrader/new_add_table.go:

    • The changes include modifying the CreateViewSql to create a view with specific columns from the PROCESSLIST() function. The change uses fmt.Sprintf to dynamically construct the SQL query based on the sysview.InformationDBConst.
    • Suggestion: Ensure that the sysview.InformationDBConst is properly defined and validated to prevent SQL injection vulnerabilities.
  2. pkg/cnservice/upgrader/upgrader.go:

    • The code has been updated to use ExecTxn method instead of manually constructing and executing SQL statements in a transaction.
    • Suggestion: This change improves code readability and maintainability. Ensure that error handling is robust and covers all possible failure scenarios.
  3. pkg/frontend/internal_executor.go:

    • The addition of the ExecTxn method to execute multiple SQL statements in a transaction.
    • Suggestion: Consider adding more detailed logging or error handling to provide better visibility into transaction execution.
  4. pkg/stream/connector/connector_test.go:

    • Addition of the ExecTxn method implementation for testing purposes.
    • Suggestion: Ensure that the testing covers various scenarios, including error cases, to validate the behavior of the ExecTxn method.
  5. pkg/util/internalExecutor/internal_executor.go:

    • Introduction of the ExecTxn method in the InternalExecutor interface to execute multiple SQL statements in a transaction.
    • Suggestion: Consider adding documentation comments to clearly explain the purpose and usage of the new method.
  6. pkg/util/metric/mometric/metric_collector_test.go:

    • Implementation of the ExecTxn method for testing purposes.
    • Suggestion: Ensure that the testing adequately covers the functionality of the ExecTxn method.
  7. pkg/util/sysview/sysview.go:

    • Modification of the CREATE VIEW SQL query to include specific columns from the PROCESSLIST() function based on InformationDBConst.
    • Suggestion: Validate the usage of InformationDBConst to prevent any potential security vulnerabilities.
  8. pkg/util/trace/impl/motrace/example/main.go:

    • Implementation of the ExecTxn method for logging purposes.
    • Suggestion: Consider adding more meaningful logging or error handling to enhance the utility of the ExecTxn method.
  9. pkg/util/trace/impl/motrace/schema_test.go:

    • Implementation of the ExecTxn method for testing purposes.
    • Suggestion: Ensure that the testing adequately covers the behavior of the ExecTxn method.

Overall Suggestions:

  • Validate user inputs and dynamically constructed SQL queries to prevent SQL injection attacks.
  • Enhance error handling to cover all possible failure scenarios and provide informative error messages.
  • Consider adding more detailed documentation and comments to explain the purpose and usage of new methods.
  • Ensure that testing

@sukki37 sukki37 merged commit 29a2f28 into matrixorigin:1.1-dev Apr 10, 2024
14 of 16 checks passed
@qingxinhome qingxinhome deleted the HandleProcesslistNotAvailable branch May 17, 2024 09:00
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.

9 participants