Improve the driver's interactions with SHOW PROCESSLIST and KILL.#215
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves the embedded driver’s integration with go-mysql-server’s ProcessList so that SHOW PROCESSLIST and KILL QUERY behave more like the server path, including creating a per-query context and ensuring query lifecycle transitions are reflected correctly.
Changes:
- Introduces per-query contexts (with unique PIDs) and wires query begin/end to the engine
ProcessList. - Updates row-set closing behavior to end queries when
Rows.Close()is called. - Refactors smoke test DB initialization/cleanup and adds a new smoke test covering
SHOW PROCESSLISTandKILL QUERY.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
conn.go |
Adds per-query context creation (beginQuery) / cleanup (endQuery) and removes connections from ProcessList on close. |
connector.go |
Builds a session/context that registers connections with the engine ProcessList. |
statement.go |
Routes statement execution through DoltConn.beginQuery and uses per-query contexts for engine calls. |
rows.go |
Ends the active query in the process list when rows are closed. |
smoke_test.go |
Refactors test DB setup/cleanup, switches many tests to t.Context(), and adds a SHOW PROCESSLIST / KILL QUERY smoke test. |
Comments suppressed due to low confidence (1)
conn.go:52
Prepareupdatesd.gmsCtx's QueryTime, but queries now execute against the per-queryqueryCtxcreated inbeginQuery. That means QueryTime updates ond.gmsCtxwill no longer affect functions that readctx.QueryTime(e.g., NOW()) when a ProcessList is present. Consider setting QueryTime onfreshCtx/queryCtxinsidebeginQuery(and updating the comment inPrepare, since the context is no longer reused for execution).
// Prepare packages up |query| as a *doltStmt so it can be executed. If multistatements mode
// has been enabled, then a *doltMultiStmt will be returned, capable of executing multiple statements.
func (d *DoltConn) Prepare(query string) (driver.Stmt, error) {
// Reuse the same ctx instance, but update the QueryTime to the current time.
// Statements are executed serially on a connection, so it's safe to reuse
// the same ctx instance and update the time.
d.gmsCtx.SetQueryTime(time.Now())
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
coffeegoddd
left a comment
There was a problem hiding this comment.
A few questions/comments.
…t we always construct doltRows with an iter.
…ok_result__ iterators. Others can be safely abandoned, which allows for NextResultSet() to terminate in-flight queries without having to wait for their full result set to drain.
…ping RowIter.Close.
…tabase(). Disable connection reuse because DoltSession has a ton of in-memory state that is not resettable.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
No description provided.