add(dictionary):会话管理增加clickhouse数据库的支持#3174
Conversation
|
你好!感谢你反馈的问题/bug，但是你的描述好像是空的，我们需要你完整的信息，这样才能帮你解决问题 如果不知道怎么写，在新建issue的时候有若干个模板可供选择，祝好！ |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3174 +/- ##
==========================================
+ Coverage 83.66% 83.71% +0.05%
==========================================
Files 138 138
Lines 23199 23277 +78
==========================================
+ Hits 19409 19487 +78
Misses 3790 3790 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e390170f9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| sql = sql.replace(/\n/g, "<br>"); | ||
| //替换所有的空格 | ||
| sql = sql.replace(/\s/g, " "); | ||
| html.push('<span>' + sql + '</span>'); |
There was a problem hiding this comment.
The ClickHouse detail formatter writes query text directly into innerHTML via html.push('<span>' + sql + '</span>') after only replacing whitespace/newlines, so any <...> content in a running query is interpreted as markup/scripts instead of text. In environments where users can submit arbitrary SQL and others view the process list, this creates an XSS path in the session-management page. Please HTML-escape the formatted query string before concatenating it into the <span>.
Useful? React with 👍 / 👎.
| for query_id in thread_ids: | ||
| # 转义 | ||
| safe_query_id = self.escape_string(str(query_id)) | ||
| kill_sql += "KILL QUERY WHERE query_id = '{}';".format(safe_query_id) |
There was a problem hiding this comment.
This path builds KILL QUERY WHERE query_id = '...' from selected query_id values, but ClickHouse system.processes.query_id can be unset/empty; in that case the generated statement becomes query_id = '', which matches all running queries with an empty ID on the node instead of only the selected row(s). That can terminate unrelated sessions, so empty IDs should be filtered/rejected (or another stable identifier like initial_query_id should be used).
Useful? React with 👍 / 👎.
| query_kind, | ||
| query | ||
| from | ||
| system.processes;""" |
There was a problem hiding this comment.
The process list is sourced from local system.processes, which only shows queries on the current node in ClickHouse Cloud/clustered deployments; this makes session-management results incomplete and can hide sessions that users expect to manage. For clustered contexts the query should aggregate across replicas (e.g., clusterAllReplicas) and align kill behavior accordingly.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ab5d97236
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| "query", | ||
| ], | ||
| rows=[ | ||
| ( |
There was a problem hiding this comment.
The new test_processlist block is syntactically incomplete: the tuple opened in rows=[( is never closed before the next decorator, so sql/engines/test_clickhouse.py cannot be imported and test collection fails with SyntaxError: '(' was never closed. This blocks the entire ClickHouse test module from running in CI, so the new behavior is currently untestable.
Useful? React with 👍 / 👎.
| rs = engine.processlist(command_type="Process") | ||
| rs = engine.tablespace() | ||
| mock_query.assert_called_once() |
There was a problem hiding this comment.
test_tablespace_default now calls both engine.processlist() and engine.tablespace() while still asserting mock_query.assert_called_once(), so this test will fail for call-count mismatch instead of validating tablespace behavior. It also makes subsequent SQL assertions ambiguous because call_args only reflects the most recent call, not the processlist SQL the test is trying to check.
Useful? React with 👍 / 👎.
| engine = ClickHouseEngine(instance=mock_instance) | ||
| engine.kill([]) | ||
| mock_execute.assert_not_called() | ||
| assert "LIMIT 0,14" in call_sql |
There was a problem hiding this comment.
In test_kill_does_not_call_execute_when_no_ids, the assertion assert "LIMIT 0,14" in call_sql references call_sql, which is never defined in that function (same for later rs assertions), so once the module parses this test raises NameError rather than testing kill behavior. These leftover assertions should be removed or replaced with local setup.
Useful? React with 👍 / 👎.
| assert rs.rows[0][0] == "qid-1" | ||
| assert rs.rows[0][2] == "127.0.0.1" |
There was a problem hiding this comment.
The expectations at the end of test_tablespace_default assert processlist-style values ("qid-1", IP address) even though rs is overwritten by engine.tablespace(), whose mocked row starts with database/table fields. This guarantees assertion failure and prevents the test from validating the intended tablespace behavior.
Useful? React with 👍 / 👎.
1 participant
clickhouse的会话查询：system.processes。
clickhouse的kill会话：KILL QUERY WHERE query_id = 'xxx'
注意：clickhouse没有类似mysql的sleep会话。