server: detect client disconnects in explicit transactions - #70343
server: detect client disconnects in explicit transactions#70343YangKeao wants to merge 1 commit into
Conversation
ti-chi-bot
Bot
added
release-note
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change replaces the TiKV client dependency, connects SQL kill signals to compatible KV variables, and extends connection-liveness handling to explicit transactions and prepared statements. Tests cover statement classification, interruption, disconnects, lock release, and transaction cleanup. ChangesConnection liveness
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Client
participant TiDBConnection
participant SQLKiller
participant TiKVTransaction
Client->>TiDBConnection: Disconnect during blocked update
TiDBConnection->>SQLKiller: Trigger connection kill signal
SQLKiller->>TiKVTransaction: Interrupt blocked statement
TiKVTransaction-->>TiDBConnection: Return query interruption
TiDBConnection-->>Client: Terminate disconnected process
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| cloud.google.com/go/storage => cloud.google.com/go/storage v1.39.1 | ||
| github.com/go-ldap/ldap/v3 => github.com/YangKeao/ldap/v3 v3.4.5-0.20230421065457-369a3bab1117 | ||
| github.com/pingcap/tidb/pkg/parser => ./pkg/parser | ||
| github.com/tikv/client-go/v2 => github.com/YangKeao/client-go/v2 v2.0.1-0.20260804124005-da5feb949c36 |
There was a problem hiding this comment.
Will replace it after tikv/client-go#2042 is merged.
84be5cf to
d34988c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/server/tests/commontest/tidb_test.go`:
- Around line 3703-3712: Update the transaction verification in the test around
the existing ExecContext call to increment v by 2 rather than overwrite it with
2, so a prior incorrect commit of v = 1 yields 3. Keep the subsequent
QueryRowContext assertions aligned with the intended rollback result of row1 = 2
and row2 = 0.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 88f7655d-e2d7-4d73-ae60-13d0343b30f9
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
DEPS.bzlgo.modpkg/server/conn.gopkg/server/conn_stmt.gopkg/server/conn_stmt_test.gopkg/server/tests/commontest/tidb_test.gopkg/sessionctx/variable/session.go
| ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) | ||
| defer cancel() | ||
| _, err = dbt.GetDB().ExecContext(ctx, "update "+tableName+" set v = 2 where id = 1") | ||
| require.NoError(t, err) | ||
|
|
||
| var row1, row2 int | ||
| err = dbt.GetDB().QueryRowContext(context.Background(), "select sum(if(id = 1, v, 0)), sum(if(id = 2, v, 0)) from "+tableName).Scan(&row1, &row2) | ||
| require.NoError(t, err) | ||
| require.Equal(t, 2, row1) | ||
| require.Equal(t, 0, row2) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not mask a failed rollback.
Line 3705 overwrites v with 2. If the disconnected transaction incorrectly commits its earlier v = 1, the final value is still 2 and this test passes. Use an increment so an incorrect commit produces 3.
Proposed test fix
- _, err = dbt.GetDB().ExecContext(ctx, "update "+tableName+" set v = 2 where id = 1")
+ _, err = dbt.GetDB().ExecContext(ctx, "update "+tableName+" set v = v + 2 where id = 1")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) | |
| defer cancel() | |
| _, err = dbt.GetDB().ExecContext(ctx, "update "+tableName+" set v = 2 where id = 1") | |
| require.NoError(t, err) | |
| var row1, row2 int | |
| err = dbt.GetDB().QueryRowContext(context.Background(), "select sum(if(id = 1, v, 0)), sum(if(id = 2, v, 0)) from "+tableName).Scan(&row1, &row2) | |
| require.NoError(t, err) | |
| require.Equal(t, 2, row1) | |
| require.Equal(t, 0, row2) | |
| ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) | |
| defer cancel() | |
| _, err = dbt.GetDB().ExecContext(ctx, "update "+tableName+" set v = v + 2 where id = 1") | |
| require.NoError(t, err) | |
| var row1, row2 int | |
| err = dbt.GetDB().QueryRowContext(context.Background(), "select sum(if(id = 1, v, 0)), sum(if(id = 2, v, 0)) from "+tableName).Scan(&row1, &row2) | |
| require.NoError(t, err) | |
| require.Equal(t, 2, row1) | |
| require.Equal(t, 0, row2) |
🧰 Tools
🪛 ast-grep (0.45.0)
[error] 3704-3704: SQL query is built by concatenating a string literal with a variable and passed to a database/sql call (Query, Exec, QueryRow, Prepare, or their Context variants). String concatenation lets attacker-controlled input alter the query structure, enabling SQL injection. Use parameterized queries with placeholders ('?' or '') and pass the values as separate arguments instead of concatenating them into the query string.
Context: dbt.GetDB().ExecContext(ctx, "update "+tableName+" set v = 2 where id = 1")
Note: [CWE-89] Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection').
(sql-injection-query-string-concat-go)
[error] 3708-3708: SQL query is built by concatenating a string literal with a variable and passed to a database/sql call (Query, Exec, QueryRow, Prepare, or their Context variants). String concatenation lets attacker-controlled input alter the query structure, enabling SQL injection. Use parameterized queries with placeholders ('?' or '') and pass the values as separate arguments instead of concatenating them into the query string.
Context: dbt.GetDB().QueryRowContext(context.Background(), "select sum(if(id = 1, v, 0)), sum(if(id = 2, v, 0)) from "+tableName)
Note: [CWE-89] Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection').
(sql-injection-query-string-concat-go)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/server/tests/commontest/tidb_test.go` around lines 3703 - 3712, Update
the transaction verification in the test around the existing ExecContext call to
increment v by 2 rather than overwrite it with 2, so a prior incorrect commit of
v = 1 yields 3. Keep the subsequent QueryRowContext assertions aligned with the
intended rollback result of row1 = 2 and row2 = 0.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/server/tests/commontest/tidb_test.go`:
- Around line 3658-3661: Update the blocker cleanup defer in the test to capture
and report errors from both blocker.ExecContext rollback and blocker.Close
instead of discarding them. Ensure both cleanup operations always run, and
include actionable context for each reported failure while preserving the
existing cleanup order.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 65666cdf-0f9f-427b-92e9-5f491497b60d
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
DEPS.bzlgo.modpkg/server/conn.gopkg/server/conn_stmt.gopkg/server/conn_stmt_test.gopkg/server/tests/commontest/tidb_test.gopkg/sessionctx/variable/session.go
🚧 Files skipped from review as they are similar to previous changes (6)
- pkg/server/conn.go
- pkg/server/conn_stmt.go
- go.mod
- pkg/server/conn_stmt_test.go
- DEPS.bzl
- pkg/sessionctx/variable/session.go
| defer func() { | ||
| _, _ = blocker.ExecContext(context.Background(), "rollback") | ||
| _ = blocker.Close() | ||
| }() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Report blocker cleanup failures.
Line 3659 discards the rollback error. Line 3660 discards the close error. If either operation fails, the test can leave the row-2 lock holder active and conceal the cleanup failure. Run both cleanup operations, then report both errors.
Proposed fix
defer func() {
- _, _ = blocker.ExecContext(context.Background(), "rollback")
- _ = blocker.Close()
+ _, rollbackErr := blocker.ExecContext(context.Background(), "rollback")
+ closeErr := blocker.Close()
+ if rollbackErr != nil {
+ t.Errorf("rollback blocker: %v", rollbackErr)
+ }
+ if closeErr != nil {
+ t.Errorf("close blocker: %v", closeErr)
+ }
}()As per coding guidelines, “Keep error handling actionable and contextual; do not silently swallow errors.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| defer func() { | |
| _, _ = blocker.ExecContext(context.Background(), "rollback") | |
| _ = blocker.Close() | |
| }() | |
| defer func() { | |
| _, rollbackErr := blocker.ExecContext(context.Background(), "rollback") | |
| closeErr := blocker.Close() | |
| if rollbackErr != nil { | |
| t.Errorf("rollback blocker: %v", rollbackErr) | |
| } | |
| if closeErr != nil { | |
| t.Errorf("close blocker: %v", closeErr) | |
| } | |
| }() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/server/tests/commontest/tidb_test.go` around lines 3658 - 3661, Update
the blocker cleanup defer in the test to capture and report errors from both
blocker.ExecContext rollback and blocker.Close instead of discarding them.
Ensure both cleanup operations always run, and include actionable context for
each reported failure while preserving the existing cleanup order.
Source: Coding guidelines
d34988c to
131001e
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #70343 +/- ##
================================================
- Coverage 76.3277% 73.4044% -2.9234%
================================================
Files 2041 2080 +39
Lines 558363 583797 +25434
================================================
+ Hits 426186 428533 +2347
- Misses 131277 154759 +23482
+ Partials 900 505 -395
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/server/tests/commontest/tidb_test.go`:
- Around line 3699-3701: Update the disconnect verification around
processlistCountByInfo to query the captured processID and assert that no
processlist row remains for that session before executing fallback cleanup. Keep
the existing statement-info check only as a fallback, ensuring the assertion
specifically detects removal of the disconnected process ID rather than merely
completion of its statement.
- Around line 3631-3642: Update the deferred cleanup around conn.Close and
stmt.Close in the prepared-statement test path to inspect and propagate
unexpected close errors with resource-specific context. Only suppress the
explicitly expected error that occurs after the raw net.Conn is closed; do not
discard other failures, while preserving cleanup ordering and existing test
assertions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fa6fbe22-6b47-4210-818b-32ade334e440
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
DEPS.bzlgo.modpkg/server/conn.gopkg/server/conn_stmt.gopkg/server/conn_stmt_test.gopkg/server/tests/commontest/tidb_test.gopkg/sessionctx/variable/session.gotests/realtikvtest/pessimistictest/pessimistic_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- pkg/server/conn.go
- pkg/sessionctx/variable/session.go
- go.mod
- pkg/server/conn_stmt_test.go
- pkg/server/conn_stmt.go
- DEPS.bzl
| defer func() { | ||
| _ = conn.Close() | ||
| }() | ||
|
|
||
| blockedSQL := fmt.Sprintf("update %s set v = 2 where id = 2", tableName) | ||
| var stmt *sql.Stmt | ||
| if prepared { | ||
| stmt, err = conn.PrepareContext(context.Background(), blockedSQL) | ||
| require.NoError(t, err) | ||
| defer func() { | ||
| _ = stmt.Close() | ||
| }() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the relevant helper and nearby tests.
file="pkg/server/tests/commontest/tidb_test.go"
if [ -f "$file" ]; then
wc -l "$file"
sed -n '3590,3675p' "$file" | nl -ba -v3590
echo '--- usages/helper definition snippets ---'
rg -n "func .*Prepare|PrepareContext|Close.*conn|conn\.Close|stmt\.Close|blockedSQL|TestClientDisconnectKills" "$file"
else
echo "file missing"
fi
echo '--- go files under commontest (if needed) ---'
find pkg/server/tests/commontest -maxdepth 1 -type f | sortRepository: pingcap/tidb
Length of output: 236
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="pkg/server/tests/commontest/tidb_test.go"
echo '--- relevant lines ---'
awk 'NR>=3590 && NR<=3675 { printf "%8d\t%s\n", NR, $0 }' "$file"
echo '--- related search results ---'
grep -nE "conn\.Close|stmt\.Close|PrepareContext|blockedSQL|TestClientDisconnectKills|defer func\(\)" "$file" | sed -n '1,120p'
echo '--- surrounding function outline around TestClientDisconnectKillsExplicitTxn ---'
grep -n "func TestClientDisconnectKillsExplicitTxn\|func .*connectAndHandleBlocked" "$file" | sed -n '1,40p'Repository: pingcap/tidb
Length of output: 6160
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="pkg/server/tests/commontest/tidb_test.go"
echo '--- continuation after raw connection extraction ---'
awk 'NR>=3665 && NR<=3725 { printf "%8d\t%s\n", NR, $0 }' "$file"
echo '--- getRawNetConn definition ---'
grep -n -A40 "func getRawNetConn" "$file"
echo '--- other conn/stmt close error handling patterns in file ---'
grep -nE "(require\.NoError\(t, conn\.Close\(\)|terror\.Call\(conn\.Close\)|_ = conn\.Close\(\)|stmt\.Close\(\))" "$file" | sed -n '1,120p'
echo '--- deterministic close-error context probe for discarded errors in source ---'
python3 - <<'PY'
from pathlib import Path
text = Path("pkg/server/tests/commontest/tidb_test.go").read_text()
for needle in ["_ = conn.Close()", "_ = stmt.Close()", "conn.Close()", "stmt.Close()"]:
print(needle, "=" , text.count(needle))
PYRepository: pingcap/tidb
Length of output: 4636
🌐 Web query:
Go database/sql Conn Close after net.Conn Close expected error documentation
💡 Result:
In Go's database/sql package, calling Conn.Close after the underlying net.Conn has already been closed is generally handled gracefully by the driver, though it may return an error depending on the driver's implementation. 1. Expected Behavior: When you call Conn.Close, the database/sql package invokes the driver's Close method [1][2]. If the underlying network connection is already closed, the driver's attempt to perform a final network-level close or cleanup may encounter an error (e.g., "connection already closed" or similar) [2]. 2. Error Handling: The Go standard library documentation for database/sql/driver does not strictly forbid returning an error from Close if the connection is already invalid [1]. Many production drivers, such as those for PostgreSQL (like pgx), include network operations within their Close implementation; if the socket is unreachable or closed, that operation may return an error to the caller [2]. 3. Connection Pool Management: If you are using a pool via *sql.DB, the pool manages the lifecycle of these connections. The database/sql package is designed to be resilient to stale connections, often surfacing them as "bad connection" or "connection already closed" errors when they are pulled from the pool and used [3][4]. 4. Best Practices: You do not typically need to manually call Close on an individual connection unless you are using sql.Conn for a dedicated, continuous session [5][6]. For standard query patterns, the sql.DB handle manages connection recycling automatically [7][8]. If you do manage a dedicated sql.Conn, you should always ensure Conn.Close is called, but be prepared for it to return an error if the underlying state has changed or if the connection was already terminated by the server or a network event [5][6][2]. In summary, seeing an error from Conn.Close after a network failure is common and expected behavior, not necessarily an indication of a bug in your code, provided you are handling the potential error returned by that Close call.
Citations:
- 1: https://pkg.go.dev/database/sql/driver@go1.26.5
- 2: database/sql: no way to protect driver.Connector.Connect(), driver.Conn.Close(), driver.Stmt.Close() from blocking golang/go#38185
- 3: https://deadends.dev/go/sql-conn-busy/
- 4: database/sql: few usable connections in connection pool after network packet loss event golang/go#64614
- 5: https://pkg.go.dev/database/sql
- 6: https://pkg.go.dev/database/sql@go1.26.5
- 7: https://go.dev/doc/database/manage-connections
- 8: https://go.dev/doc/database/open-handle
Handle client-resource cleanup errors instead of discarding them.
The deferred conn.Close and stmt.Close ignore failure cases. Return unexpected close errors with resource context; only swallow errors that are explicitly known to be expected after the raw net.Conn is closed.
Do this in pkg/server/tests/commontest/tidb_test.go:3631-3641.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/server/tests/commontest/tidb_test.go` around lines 3631 - 3642, Update
the deferred cleanup around conn.Close and stmt.Close in the prepared-statement
test path to inspect and propagate unexpected close errors with
resource-specific context. Only suppress the explicitly expected error that
occurs after the raw net.Conn is closed; do not discard other failures, while
preserving cleanup ordering and existing test assertions.
Source: Coding guidelines
| require.Eventually(t, func() bool { | ||
| return processlistCountByInfo(t, dbt, pattern) == 0 | ||
| }, 5*time.Second, 50*time.Millisecond) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map relevant helper declarations before inspecting implementations.
ast-grep outline pkg/server/tests/commontest/tidb_test.go \
--items all --type function --match 'processlist|cleanupProcess'
# Expect either a process-ID absence check or helper semantics that prove it.
rg -n -C 12 --type go \
'func\s+(processlist(?:CountByInfo|IDByInfo)|cleanupProcessByID)\b|(?:processlist(?:CountByInfo|IDByInfo)|cleanupProcessByID)\(' \
pkg/serverRepository: pingcap/tidb
Length of output: 10654
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the disconnected statement test helper and the helper bodies around cleanupProcessByID.
sed -n '3548,3775p' pkg/server/tests/commontest/tidb_test.go | cat -nRepository: pingcap/tidb
Length of output: 8217
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the remaining cleanupProcessByID implementation and nearby tests for the expected cleanup path.
sed -n '3768,3802p' pkg/server/tests/commontest/tidb_test.go | cat -n
rg -n -C 8 'cleanupProcessByID\(t, dbt\.GetDB\(\), processID\)' pkg/server/tests/commontest/tidb_test.goRepository: pingcap/tidb
Length of output: 2377
Assert that the captured process ID is absent after disconnect.
processlistCountByInfo only checks information_schema.processlist.info, so it can pass after statement completion while the disconnected session remains. Use the captured processID and assert no processlist row exists for that ID before the fallback cleanup.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/server/tests/commontest/tidb_test.go` around lines 3699 - 3701, Update
the disconnect verification around processlistCountByInfo to query the captured
processID and assert that no processlist row remains for that session before
executing fallback cleanup. Keep the existing statement-info check only as a
fallback, ensuring the assertion specifically detects removal of the
disconnected process ID rather than merely completion of its statement.
3409ddb to
c55f7ed
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/server/tests/commontest/tidb_test.go`:
- Around line 3683-3685: Update the fallback cleanup in RunTests after processID
is captured so cleanupProcessByID’s kill-query work executes while db remains
open, rather than being deferred through t.Cleanup until after db closes. Use a
local cleanup helper and defer or invoke it before the test callback returns,
preserving the existing process lookup and cleanup behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 900b7757-0731-4fd8-afbb-1ec8196f4e69
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
DEPS.bzlgo.modpkg/server/conn.gopkg/server/conn_stmt.gopkg/server/conn_stmt_test.gopkg/server/tests/commontest/tidb_test.gopkg/sessionctx/variable/session.gotests/realtikvtest/pessimistictest/pessimistic_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/realtikvtest/pessimistictest/pessimistic_test.go
- go.mod
- pkg/sessionctx/variable/session.go
- pkg/server/conn_stmt.go
- pkg/server/conn_stmt_test.go
- pkg/server/conn.go
|
/test check-dev |
|
/hold |
ti-chi-bot
Bot
added
the
do-not-merge/hold
9e96bb9 to
4fe9a0b
Compare
| } | ||
| switch stmt.(type) { | ||
| explicitTxn := !sessVars.IsAutocommit() || sessVars.InTxn() | ||
| switch stmt := stmt.(type) { |
There was a problem hiding this comment.
Other statements like DDL are still ignored here to keep existing behavior.
If the DDL is canceled because of the misconfiguration of keepalive, it might be annoying.
4fe9a0b to
3bd2c05
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
YangKeao
changed the title
YangKeao
changed the title
ddfd3d6 to
b3899a0
Compare
YangKeao
changed the title
b3899a0 to
e4f46d3
Compare
Signed-off-by: Yang Keao <yangkeao@chunibyo.icu>
e4f46d3 to
6516fba
Compare
|
@YangKeao: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
1 participant
What problem does this PR solve?
Issue Number: close #68682
Problem Summary:
When a client disconnects while a statement in an explicit transaction is blocked in TiKV, TiDB cannot read the next command to notice the closed connection. The statement can keep retrying and retain transaction locks long after the client has gone away.
What changed and how does it work?
SQLKilleras client-go's kill-signal handler so TiKV retry checkpoints can cooperatively run the existing signal and connection-liveness checks.ExecuteStmtfor DML, and forDOand locking reads in explicit transactions. Ordinary reads continue to install it while writing their result sets, and DDL and transaction-control statements remain excluded from early cancellation.EXECUTEand binaryCOM_STMT_EXECUTEto the underlying prepared AST before deciding whether the probe is needed.BEGINandautocommit=0transactions with both COM_QUERY and binary prepared-statement protocols.Depends on tikv/client-go#2042. The client-go fork replacement is temporary until that PR is merged.
Check List
Tests
Unit and integration tests:
Manual test with a real TiKV cluster:
The manual matrix closed the client TCP connection while statements were blocked and covered COM_QUERY, SQL
PREPARE/EXECUTE, binaryCOM_STMT_PREPARE/COM_STMT_EXECUTEwith parameters,BEGIN,autocommit=0, autocommit DML, INSERT/UPDATE/DELETE,SELECT ... FOR UPDATE,FOR UPDATE WAIT,FOR UPDATE NOWAIT,LOCK IN SHARE MODE,DO SLEEP(), ordinarySELECT SLEEP(), and DDL waiting for metadata locks. All 20 cases passed. Explicit transactions were rolled back and released their earlier locks in about one second. Disconnected DDL sessions were intentionally not killed and completed after their metadata-lock blocker was released.make bazel_preparewas also attempted, but this machine does not have thebazelexecutable installed (make: bazel: No such file or directory).Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
Bug Fixes
Tests