[fix](connect) Align COM_RESET_CONNECTION behavior with MySQL#63884
Merged
morrySnow merged 8 commits intoJun 3, 2026
Conversation
COM_RESET_CONNECTION should clear session-scoped state without losing user-level default session settings such as query and insert timeout. Key changes: - Reset session variables, user variables, prepared statements, catalog/database state, running query, and return rows on COM_RESET_CONNECTION. - Reapply user default query_timeout and insert_timeout after rebuilding SessionVariable. - Add a regression helper to send COM_RESET_CONNECTION directly through the MySQL protocol. Unit Test: - ConnectContextTest - test_reset_connection_session_variable
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 31506 ms |
Contributor
TPC-DS: Total hot run time: 173055 ms |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
Author
|
run nonConcurrent |
Contributor
FE Regression Coverage ReportIncrement line coverage |
COM_RESET_CONNECTION should reset per-connection state without clearing the current database, and cleanup failures should be returned to clients instead of silently succeeding. Key changes: - Preserve current catalog and database across reset connection. - Reset user variables, prepared statements, running query state, insert result, temporary tables, and transaction state. - Return autocommit server status on reset OK and MySQL-compatible prepared statement errors. Unit Test: - ConnectContextTest - test_reset_connection_session_variable - FE checkstyle validate
Contributor
Author
|
Updated the PR to align Changes added in the latest commit:
Validation:
|
Contributor
Author
|
run buildall |
yujun777
changed the title
yujun777
changed the title
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review opinion: approve/comment only. I did not find blocking correctness issues in the PR scope.
Critical checkpoint conclusions:
- Goal and tests: The change aligns COM_RESET_CONNECTION with the intended behavior by resetting per-connection state, preserving current catalog/database, cleaning prepared/user/session state, and returning reset failures. FE unit tests and a regression test cover the main session-variable and database-preservation path.
- Scope and clarity: The implementation is focused on ConnectContext reset handling, MySQL command handling, and targeted tests.
- Concurrency and locking: No new shared cross-thread state or lock ordering issue was identified. Reset runs in the connection processing path for its own ConnectContext.
- Lifecycle/static initialization: No new static/global initialization dependency was introduced.
- Configuration: No new configuration item was added.
- Compatibility/protocol: The COM_RESET_CONNECTION response and unknown prepared statement error are more MySQL-compatible. No FE-BE protocol or persisted metadata compatibility concern was identified.
- Parallel paths: The MySQL command path and direct ConnectContext reset path were both updated where applicable.
- Conditional checks/error handling: Transaction rollback and temporary table cleanup failures are converted to UserException/DdlException and returned to the client instead of silently succeeding.
- Test coverage: Coverage exists for session variable reset, user defaults, prepared/user state cleanup, insert result cleanup, current DB preservation, OK autocommit status, and reset error propagation. I did not run tests in this review environment.
- Observability: Existing logging around temporary table cleanup remains sufficient for this change.
- Transaction/persistence/data correctness: No persisted metadata format or visible-version path is changed. Reset aborts the active transaction and reports failure if abort fails.
- Performance: No hot-path performance concern was found; work is limited to explicit connection reset.
- User focus: No additional user-provided review focus was present.
morrySnow
reviewed
Jun 1, 2026
morrySnow
left a comment
morrySnow
left a comment
Contributor
There was a problem hiding this comment.
Reviewed reset-connection state handling.
Comment on lines
+159
to
+161
| if (ctx.getSessionVariable().autoCommit) { | ||
| ctx.getState().serverStatus |= MysqlServerStatusFlag.SERVER_STATUS_AUTOCOMMIT; | ||
| } |
Contributor
There was a problem hiding this comment.
这个可能可以不改，因为第一次连接的时候，也没这个 auto commit
Contributor
Author
There was a problem hiding this comment.
Removed the autocommit server status update from COM_RESET_CONNECTION. Doris currently does not set this flag on the initial OK packet either, so reset now keeps the OK packet status behavior consistent with the existing connection path.
| ctx.clearLastDBOfCatalog(); | ||
| ctx.getState().setOk(); | ||
| try { | ||
| ctx.resetConnection(); |
Contributor
There was a problem hiding this comment.
resetConnection() clears preparedStatementContextMap, but COM_STMT_RESET still never reads the statement id and handleStmtReset() always returns OK. After a client sends COM_RESET_CONNECTION, mysql_stmt_reset() on one of the old handles should receive an ERR packet because the statement id is no longer valid; with this path it succeeds and the client only sees the failure later on execute. Please parse the stmt id in COM_STMT_RESET and return ERR_UNKNOWN_STMT_HANDLER when ctx.getPreparedStementContext(...) is null, with the message target mysqld_stmt_reset.
Contributor
Author
There was a problem hiding this comment.
Fixed. COM_STMT_RESET now parses the statement id in the MySQL packet path and returns ERR_UNKNOWN_STMT_HANDLER when the prepared statement context is missing, using mysqld_stmt_reset in the error message.
| preparedQuerys.clear(); | ||
| preparedStatementContextMap.clear(); | ||
| preparedStmtId = Integer.MIN_VALUE; | ||
| runningQuery = null; |
Contributor
There was a problem hiding this comment.
This clears the textual runningQuery, but leaves queryId from the previous statement. An idle connection that has just been reset can still show the old query id in SHOW PROCESSLIST / connection profile APIs, which is not a fresh-session state. Please clear the current query id as part of reset before returning OK.
Contributor
Author
There was a problem hiding this comment.
Fixed. resetConnection() now clears the current query id by calling resetQueryId(), so a reset idle connection no longer exposes the previous statement query id in processlist or connection profile APIs.
Contributor
TPC-H: Total hot run time: 29231 ms |
Contributor
TPC-DS: Total hot run time: 170124 ms |
Align reset connection handling with Doris' existing MySQL OK packet behavior and make prepared statement reset report cleared statement ids immediately. Key changes: - Do not set autocommit server status in COM_RESET_CONNECTION OK packets - Clear query id during connection reset - Parse COM_STMT_RESET statement id and return unknown statement errors for cleared statements Unit Test: - ConnectContextTest
Contributor
Author
|
run buildall |
### What problem does this PR solve? Issue Number: close apache#63884 Related PR: apache#63884 Problem Summary: Extend the COM_RESET_CONNECTION regression coverage to verify that reset keeps the current database while restoring session state, clears user variables, and invalidates server-side prepared statements from the previous session state. ### Release note None ### Check List (For Author) - Test: Regression test - Attempted ./run-regression-test.sh --run -d query_p0/session_variable -s test_reset_connection_session_variable, but the local FE mysql port 127.0.0.1:59130 timed out before the case could complete. - Behavior changed: No - Does this need documentation: No
Extend the reset connection regression to cover user variables, processlist state, and server-side prepared statements after COM_RESET_CONNECTION. Key changes: - Send COM_RESET_CONNECTION directly in the regression case. - Verify reset clears user variables, processlist QueryId/Info, and server prepared statement handlers. Unit Test: - ./run-regression-test.sh --run -d query_p0/session_variable -s test_reset_connection_session_variable
Contributor
Author
|
run buildall |
Use a named constant for the initial prepared statement id and keep temporary table map cleanup scoped to connections that have temporary tables. Key changes: - Add INITIAL_PREPARED_STMT_ID for prepared statement id initialization and reset - Move temporary table map clear into the non-empty cleanup branch Unit Test: - ConnectContextTest
Contributor
Author
|
run buildall |
1 similar comment
Contributor
Author
|
run buildall |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
Author
|
run buildall |
github-actions
Bot
removed
the
approved
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 29284 ms |
Contributor
TPC-DS: Total hot run time: 168846 ms |
Contributor
TPC-H: Total hot run time: 29100 ms |
Contributor
TPC-DS: Total hot run time: 169550 ms |
Contributor
|
/review |
Contributor
|
OpenCode automated review failed and did not complete. Error: Review step was skipped (possibly timeout or cancelled) Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
There was a problem hiding this comment.
Review conclusion: no additional blocking issues found in the current PR diff after accounting for the existing inline review threads.
Critical checkpoint conclusions:
- Goal/test: The PR implements COM_RESET_CONNECTION cleanup for session variables, user variables, prepared statements, temporary tables, transaction state, query identity, trace id, insert result, command, and return rows. Added FE unit tests and a regression test cover the main reset behavior and stale prepared statement handles.
- Scope: The current GitHub PR diff is focused on FE connection reset handling and test support.
- Concurrency/lifecycle: No new shared concurrent state or non-obvious lifecycle issue was found in the reviewed paths. Temporary-table cleanup now snapshots session bookkeeping before dropping tables.
- Configuration/compatibility: No new config items or storage/protocol format changes were introduced. The COM_STMT_RESET/EXECUTE error code change aligns stale server-prepared-statement behavior with unknown statement handler errors.
- Parallel paths: COM_RESET_CONNECTION and COM_STMT_RESET packet paths were reviewed; existing review threads already covered query id, last query id, trace id, statement id monotonicity, and temporary-table cleanup.
- Error handling: Reset failures are converted to query-state errors; no unchecked Status-style omission applies in this Java path.
- Test coverage: Unit and regression coverage exists for session variable reset, user variable clearing, processlist query id cleanup, stale prepared statement execution, reset failure handling, trace id cleanup, and multiple temporary tables. I did not run tests in this review runner.
- Observability/performance: No additional observability or performance concern was identified for this narrow reset path.
User focus: no additional user-provided review focus was specified.
morrySnow
approved these changes
Jun 3, 2026
github-actions Bot
pushed a commit
that referenced
this pull request
Jun 3, 2026
### What problem does this PR solve? `COM_RESET_CONNECTION` was accepted by Doris, but its behavior was not compatible with MySQL. The previous implementation cleared the current catalog/database state and returned OK after only a partial reset. This could make pooled clients, such as C# MySqlConnector with `ConnectionReset=True`, fail later unqualified SQL with `Current database is not set`. Other session-scoped state, including user variables and prepared statements, also needed to be reset consistently. ### What is changed? - Preserve the current catalog/database state across `COM_RESET_CONNECTION` so pooled connections can continue using the selected database. - Reset session variables, user variables, prepared statements, running query state, insert result, command state, and returned row count. - Roll back transaction state during reset and return an error if rollback fails. - Drop temporary tables during reset and return an error if cleanup fails. - Return OK with the autocommit server status when reset succeeds. - Return the MySQL-compatible unknown prepared statement error when executing a statement cleared by reset. - Extend regression and FE unit coverage for reset behavior, error handling, and current database preservation.
This was referenced Jun 3, 2026
github-actions Bot
pushed a commit
that referenced
this pull request
Jun 3, 2026
### What problem does this PR solve? `COM_RESET_CONNECTION` was accepted by Doris, but its behavior was not compatible with MySQL. The previous implementation cleared the current catalog/database state and returned OK after only a partial reset. This could make pooled clients, such as C# MySqlConnector with `ConnectionReset=True`, fail later unqualified SQL with `Current database is not set`. Other session-scoped state, including user variables and prepared statements, also needed to be reset consistently. ### What is changed? - Preserve the current catalog/database state across `COM_RESET_CONNECTION` so pooled connections can continue using the selected database. - Reset session variables, user variables, prepared statements, running query state, insert result, command state, and returned row count. - Roll back transaction state during reset and return an error if rollback fails. - Drop temporary tables during reset and return an error if cleanup fails. - Return OK with the autocommit server status when reset succeeds. - Return the MySQL-compatible unknown prepared statement error when executing a statement cleared by reset. - Extend regression and FE unit coverage for reset behavior, error handling, and current database preservation.
This was referenced Jun 3, 2026
morningman
pushed a commit
that referenced
this pull request
Jun 24, 2026
### What problem does this PR solve? `COM_RESET_CONNECTION` was accepted by Doris, but its behavior was not compatible with MySQL. The previous implementation cleared the current catalog/database state and returned OK after only a partial reset. This could make pooled clients, such as C# MySqlConnector with `ConnectionReset=True`, fail later unqualified SQL with `Current database is not set`. Other session-scoped state, including user variables and prepared statements, also needed to be reset consistently. ### What is changed? - Preserve the current catalog/database state across `COM_RESET_CONNECTION` so pooled connections can continue using the selected database. - Reset session variables, user variables, prepared statements, running query state, insert result, command state, and returned row count. - Roll back transaction state during reset and return an error if rollback fails. - Drop temporary tables during reset and return an error if cleanup fails. - Return OK with the autocommit server status when reset succeeds. - Return the MySQL-compatible unknown prepared statement error when executing a statement cleared by reset. - Extend regression and FE unit coverage for reset behavior, error handling, and current database preservation.
morningman
pushed a commit
to yujun777/doris
that referenced
this pull request
Jun 24, 2026
### What problem does this PR solve? Issue Number: close apache#63884 Related PR: apache#63884 Problem Summary: Extend the COM_RESET_CONNECTION regression coverage to verify that reset keeps the current database while restoring session state, clears user variables, and invalidates server-side prepared statements from the previous session state. ### Release note None ### Check List (For Author) - Test: Regression test - Attempted ./run-regression-test.sh --run -d query_p0/session_variable -s test_reset_connection_session_variable, but the local FE mysql port 127.0.0.1:59130 timed out before the case could complete. - Behavior changed: No - Does this need documentation: No
morningman
pushed a commit
to yujun777/doris
that referenced
this pull request
Jun 24, 2026
Backporting PR apache#63884 to branch-4.1 leaves ConnectContextTest incompatible with the branch's existing test setup. The new reset-connection cases need local Mockito mocks for catalog-related dependencies, and the env stubs must be set before ConnectContext.setEnv() initializes the default catalog. Key changes: - add missing Mockito and catalog imports used by the new reset-connection tests - use local Mockito mocks in reset-connection test cases instead of class-level JMockit mocks - stub internal catalog access before calling ConnectContext.setEnv() in the new tests Unit Test: - run-fe-ut.sh --run ConnectContextTest
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
COM_RESET_CONNECTIONwas accepted by Doris, but its behavior was not compatible with MySQL. The previous implementation cleared the current catalog/database state and returned OK after only a partial reset. This could make pooled clients, such as C# MySqlConnector withConnectionReset=True, fail later unqualified SQL withCurrent database is not set. Other session-scoped state, including user variables and prepared statements, also needed to be reset consistently.What is changed?
COM_RESET_CONNECTIONso pooled connections can continue using the selected database.Check List
tools/fast-compile-fe.shtools/fast-compile-fe.sh --test ConnectContextTest.javarun-fe-ut.sh --run ConnectContextTestmvn -f fe/pom.xml -pl fe-core -am validate -DskipTests -Dmaven.build.cache.enabled=falsetest_reset_connection_session_variable