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

add debug info for panic #16635

Merged
merged 7 commits into from
Jun 5, 2024

Conversation

daviszhen
Copy link
Contributor

@daviszhen daviszhen commented Jun 4, 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 ##16548

What this PR does / why we need it:

issue上的问题是事务状态异常。

在出问题的调用栈上,增加事务状态的检测逻辑。

txnIsValid 判断事务状态是否异常。


PR Type

Bug fix, Tests


Description

  • Added transaction validity checks using disttae.CheckTxnIsValid in multiple functions across the codebase to handle transaction state exceptions.
  • Simplified conditional checks for specific statement types.
  • Added mock expectations for transaction status in various test cases to ensure proper testing of transaction states.
  • Introduced utility functions txnIsValid and CheckTxnIsValid for consistent transaction validation.

Changes walkthrough 📝

Relevant files
Bug fix
6 files
back_exec.go
Add transaction validity check and simplify statement type check.

pkg/frontend/back_exec.go

  • Added transaction validity check using disttae.CheckTxnIsValid before
    executing statements.
  • Simplified conditional check for tree.Execute statement type.
  • +8/-6     
    back_result_row_stmt.go
    Add transaction validity check in result row statements. 

    pkg/frontend/back_result_row_stmt.go

  • Added transaction validity check using disttae.CheckTxnIsValid before
    executing result row statements.
  • +7/-0     
    back_status_stmt.go
    Add transaction validity check in status statements.         

    pkg/frontend/back_status_stmt.go

  • Added transaction validity check using disttae.CheckTxnIsValid before
    executing status statements.
  • +8/-0     
    mysql_cmd_executor.go
    Add transaction validity check in statement execution functions.

    pkg/frontend/mysql_cmd_executor.go

  • Added transaction validity check using disttae.CheckTxnIsValid in
    multiple functions handling statement execution.
  • +11/-0   
    compile.go
    Add transaction validity check in table scan data source compilation.

    pkg/sql/compile/compile.go

  • Added transaction validity check using disttae.CheckTxnIsValid in
    compileTableScanDataSource.
  • +10/-1   
    engine.go
    Use `txnIsValid` function for transaction validation.       

    pkg/vm/engine/disttae/engine.go

  • Replaced direct workspace checks with txnIsValid function to validate
    transactions.
  • +21/-18 
    Tests
    3 files
    mysql_cmd_executor_test.go
    Add mock expectation for transaction status in tests.       

    pkg/frontend/mysql_cmd_executor_test.go

  • Added mock expectation for Status method returning
    txn.TxnStatus_Active in multiple test cases.
  • +7/-0     
    mysql_protocol_test.go
    Add mock expectation for transaction status in `TestKill`.

    pkg/frontend/mysql_protocol_test.go

  • Added mock expectation for Status method returning
    txn.TxnStatus_Active in TestKill.
  • +1/-0     
    session_test.go
    Add mock expectation for transaction status in `TestSession_Migrate`.

    pkg/frontend/session_test.go

  • Added mock expectation for Status method returning
    txn.TxnStatus_Active in TestSession_Migrate.
  • +1/-0     
    Enhancement
    1 files
    util.go
    Add utility functions for transaction validation.               

    pkg/vm/engine/disttae/util.go

  • Added txnIsValid and CheckTxnIsValid functions to validate transaction
    operators.
  • +37/-4   

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

    @daviszhen daviszhen changed the title update add debug info for panic Jun 4, 2024
    @codiumai-pr-agent-pro codiumai-pr-agent-pro bot changed the title add debug info for panic update Jun 4, 2024
    @mergify mergify bot requested a review from sukki37 June 4, 2024 08:27
    @mergify mergify bot added the kind/bug Something isn't working label Jun 4, 2024
    @matrix-meow matrix-meow added the size/M Denotes a PR that changes [100,499] lines label Jun 4, 2024
    Copy link

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

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    3, because the PR involves multiple files and changes related to transaction validity checks across various components. The changes are not overly complex but require a good understanding of the transaction flow and error handling mechanisms in the system.

    🧪 Relevant tests

    Yes

    ⚡ Possible issues

    Redundant Code: The txnIsValid function checks for txnOp == nil and ws == nil which might be redundant if these conditions are guaranteed not to occur by the design of the transaction operator interface.

    Error Handling: In some places, the error from CheckTxnIsValid is directly returned without additional context or logging, which might make debugging more difficult if the error is not self-explanatory.

    🔒 Security concerns

    No

    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 bug
    Add a nil check for txnOp before calling CheckTxnIsValid to prevent potential runtime errors

    To avoid potential nil pointer dereference, ensure that txnOp is not nil before calling
    CheckTxnIsValid.

    pkg/frontend/mysql_cmd_executor.go [2315-2318]

    +if txnOp == nil {
    +    return fmt.Errorf("txnOp is nil")
    +}
     err = disttae.CheckTxnIsValid(txnOp)
     if err != nil {
         return err
     }
     
    Suggestion importance[1-10]: 9

    Why: This suggestion correctly identifies a potential nil pointer dereference, which is a critical runtime error, thus improving the robustness of the code.

    9
    Add a nil check for fPrintTxnOp before calling CheckTxnIsValid to prevent potential runtime errors

    To avoid potential nil pointer dereference, ensure that fPrintTxnOp is not nil before
    calling CheckTxnIsValid.

    pkg/frontend/back_status_stmt.go [30-33]

    +if fPrintTxnOp == nil {
    +    return fmt.Errorf("fPrintTxnOp is nil")
    +}
     err = disttae.CheckTxnIsValid(fPrintTxnOp)
     if err != nil {
         return err
     }
     
    Suggestion importance[1-10]: 9

    Why: Similar to the previous suggestion, this one correctly addresses a potential nil pointer dereference in another part of the code, enhancing its safety.

    9
    Test coverage
    Add a test case to verify behavior for different transaction statuses

    Consider adding a test case to verify the behavior when txnOperator.Status() returns a
    status other than txn.TxnStatus_Active. This will ensure that the new mock expectation is
    thoroughly tested.

    pkg/frontend/mysql_cmd_executor_test.go [99]

     txnOperator.EXPECT().Status().Return(txn.TxnStatus_Active).AnyTimes()
    +// Add a test case for a different status
    +txnOperator.EXPECT().Status().Return(txn.TxnStatus_Aborted).AnyTimes()
     
    Suggestion importance[1-10]: 8

    Why: The suggestion to add a test case for different transaction statuses is crucial for ensuring robustness and completeness in testing, especially when new behavior is introduced.

    8
    Best practice
    Add logging for errors before returning them to aid in debugging

    Consider logging the error before returning it to provide more context for debugging
    purposes.

    pkg/frontend/mysql_cmd_executor.go [2316-2318]

     if err != nil {
    +    log.Errorf("Transaction validation failed: %v", err)
         return err
     }
     
    Suggestion importance[1-10]: 8

    Why: Adding error logging before returning the error in critical transaction validation checks significantly aids in debugging and is a best practice.

    8
    Debugging
    Add logging for error conditions in the txnIsValid function

    Consider logging an error message before returning an error in the txnIsValid function to
    aid in debugging.

    pkg/vm/engine/disttae/util.go [1321-1322]

     if txnOp == nil {
    +    zap.L().Error("txnOp is nil")
         return nil, moerr.NewInternalErrorNoCtx("txnOp is nil")
     }
     
    Suggestion importance[1-10]: 7

    Why: Adding logging before returning an error can significantly aid in debugging, making this a valuable suggestion for error traceability.

    7
    Performance
    Check transaction validity once and reuse the result to improve performance

    To improve performance, consider checking the transaction validity only once and reusing
    the result in the executeStmtInBack function.

    pkg/frontend/back_exec.go [373-375]

    -err = disttae.CheckTxnIsValid(execCtx.ses.GetTxnHandler().GetTxn())
    +txn := execCtx.ses.GetTxnHandler().GetTxn()
    +err = disttae.CheckTxnIsValid(txn)
     if err != nil {
         return err
     }
     
    Suggestion importance[1-10]: 7

    Why: Optimizing the check for transaction validity to occur once and reusing the result is a good practice for performance, but the impact might be limited depending on the frequency and context of function calls.

    7
    Maintainability
    Rename txnIsValid function to validateTxn for better readability

    To improve readability and maintainability, consider renaming the txnIsValid function to
    validateTxn to better reflect its purpose.

    pkg/vm/engine/disttae/engine.go [137]

    -txn, err := txnIsValid(op)
    +txn, err := validateTxn(op)
     
    Suggestion importance[1-10]: 6

    Why: Renaming the function to validateTxn improves readability and maintainability, but it's a minor improvement and not critical to functionality.

    6
    Group import statements into standard library and third-party imports for better readability

    Group the import statements into standard library imports and third-party imports for
    better readability.

    pkg/sql/compile/util.go [20-29]

    +"fmt"
    +"time"
     
    +"github.com/matrixorigin/matrixone/pkg/catalog"
    +"github.com/matrixorigin/matrixone/pkg/container/types"
    +"github.com/matrixorigin/matrixone/pkg/pb/plan"
    +"github.com/matrixorigin/matrixone/pkg/vm/engine"
    +"github.com/matrixorigin/matrixone/pkg/vm/process"
    +plan2 "github.com/matrixorigin/matrixone/pkg/sql/plan"
     
    Suggestion importance[1-10]: 6

    Why: While the suggestion improves readability by organizing imports, it is a minor maintainability improvement and does not impact functionality or performance.

    6

    @daviszhen daviszhen changed the title update add debug info for panic Jun 4, 2024
    @mergify mergify bot merged commit 82a9d0b into matrixorigin:1.2-dev Jun 5, 2024
    16 of 18 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
    Bug fix kind/bug Something isn't working Review effort [1-5]: 3 size/M Denotes a PR that changes [100,499] lines Tests
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    9 participants