ClickHouse / ClickHouse/clickhouse-go
PrepareBatch insert-query parser does not recognize // or /* */ comments, so a commented-out INSERT hijacks the target table and column list
- Dominant language
- Go
- Stars
- 3.3k
- Forks
- 680
- Avg merge
- 2d 3h
- Merged PRs (30d)
- 14
Description
## Description
`extractInsertQueryComponents` (`batch.go:25`) parses the INSERT statement out of the user's query with
```go
var normalizeInsertQueryMatch = regexp.MustCompile(`(?i)(?:(?:--[^\n]*|#![^\n]*|#\s[^\n]*)\n\s*)*(INSERT\s+INTO\s+([^(]+)(?:\s*\([^()]*(?:\([^()]*\)[^()]*)*\))?)(?:\s*VALUES)?`)
```
The leading `(?:--…|#!…|#\s…)` group exists to skip comments so the regex does not latch onto an
`INSERT INTO` that lives inside a comment (added in #1693). But the ClickHouse lexer also accepts
`//` line comments and `/* … */` block comments, and neither is in that group. Because the regex is
unanchored, the first `INSERT INTO` *anywhere* in the string wins — so when a `//` or `/* */` comment
mentions an INSERT, the client silently parses the commented-out statement instead of the real one.
`extractInsertQueryComponents` / `extractNormalizedInsertQueryAndColumns` feed:
- `conn_batch.go:23` — native `PrepareBatch`
- `conn_http_batch.go:77` — HTTP `PrepareBatch`
- `conn_http_format.go:338` and `format.go:93` — `InsertFormat`
so the consequence is that a batch is prepared against the wrong table with the wrong column list.
If the commented-out table exists and its schema is compatible, rows land in the wrong table with no
error at all; otherwise the user gets an `UNKNOWN_TABLE` / column mismatch error naming a table they
did not ask to write to.
This mirrors ClickHouse/clickhouse-connect#925, where the same class of gap (`//`, `#`, nested block
comments, backtick identifiers, heredocs missing from the comment stripper) misclassifies queries. In
clickhouse-go the exposure is narrower — there is no `remove_sql_comments` and no `query_limit`
rewriting — but the insert-query parser has the same incomplete comment coverage.
## ClickHouse server version
Not verified against a running server: no ClickHouse instance was reachable in this environment
(nothing listening on `:8123`/`:9000`). The finding is a code-analysis result confirmed by the unit
test below, which exercises the library's own parser directly — the exact function `PrepareBatch`
calls before any bytes hit the wire. Server-side, `//` and `/* … */` are both documented ClickHouse
comment syntaxes (`SELECT 1 //x` and `SELECT 1 /* x */` both return `1`).
## Reproduction
`batch_comment_test.go` in the root package:
```go
package clickhouse
import (
"testing"
"github.com/stretchr/testify/assert"
)
func TestInsertCommentSkipping(t *testing.T) {
cases := []struct {
name string
query string
}{
{"dash-dash", "-- INSERT INTO wrong_table (a)\nINSERT INTO right_table (b, c)"},
{"hash-space", "# INSERT INTO wrong_table (a)\nINSERT INTO right_table (b, c)"},
{"hash-bang", "#! INSERT INTO wrong_table (a)\nINSERT INTO right_table (b, c)"},
{"double-slash", "// INSERT INTO wrong_table (a)\nINSERT INTO right_table (b, c)"},
{"block", "/* INSERT INTO wrong_table (a) */ INSERT INTO right_table (b, c)"},
{"block-multiline", "/*\nINSERT INTO wrong_table (a)\n*/\nINSERT INTO right_table (b, c)"},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
stmt, table, cols, err := extractInsertQueryComponents(tc.query)
t.Logf("stmt=%q table=%q cols=%v err=%v", stmt, table, cols, err)
assert.NoError(t, err)
assert.Equal(t, "right_table", table)
assert.Equal(t, []string{"b", "c"}, cols)
})
}
}
```
`go test -run TestInsertCommentSkipping -v .`
| comment style | parsed statement | table | columns | result |
| --- | --- | --- | --- | --- |
| `--` | `INSERT INTO right_table (b, c)` | `right_table` | `[b c]` | pass |
| `#` + space | `INSERT INTO right_table (b, c)` | `right_table` | `[b c]` | pass |
| `#!` | `INSERT INTO right_table (b, c)` | `right_table` | `[b c]` | pass |
| `//` | `INSERT INTO wrong_table (a)` | `wrong_table` | `[a]` | **FAIL** |
| `/* … */` | `INSERT INTO wrong_table (a)` | `wrong_table` | `[a]` | **FAIL** |
| `/* … */` multiline | `INSERT INTO wrong_table (a)` | `wrong_table` | `[a]` | **FAIL** |
Expected in all six rows: `table == "right_table"`, `cols == ["b", "c"]`.
Actual for the `//` and `/* */` rows: `table == "wrong_table"`, `cols == ["a"]`.
At the client level this means:
```go
query := "// INSERT INTO wrong_table (a)\nINSERT INTO right_table (b, c)"
batch, _ := conn.PrepareBatch(ctx, query) // prepares "INSERT INTO wrong_table (a) FORMAT Native"
batch.Append(uint64(42), "hello")
batch.Send() // targets wrong_table, not right_table
```
## Suggested fix
`batch.go:9` (and the duplicate `insertMatch` at `conn_batch.go:19`): the leading-comment group needs
a `//[^\n]*` alternative and a block-comment alternative. Given that the same function also blindly
strips `FORMAT …` (`truncateFormat`, `batch.go:10`) and ` VALUES …` (`truncateValues`, `batch.go:11`)
from anywhere in the text — including inside string literals, backtick identifiers and heredocs — the
more robust fix is the one suggested upstream: a single linear left-to-right scan that follows the
server lexer (`--`, `//`, `#` + space and `#!` line comments; nested `/* */`; single-quote,
double-quote and backtick quoting with backslash and doubled-quote escapes; heredocs) and strips
comments before the INSERT statement is matched.
Also note that `insertMatch` and `columnMatch` in `conn_batch.go:19-20` appear to be dead code —
nothing references them now that `prepareBatch` goes through
`extractNormalizedInsertQueryAndColumns` — so whatever fix lands should probably delete them rather
than keep a second copy of the pattern in sync.
## Link
Relayed from ClickHouse/clickhouse-connect#925
Contributor guide
Research direction
Start with batch.go:9-25 and the extraction functions used by conn_batch.go:23 and conn_http_batch.go:77, then run the root-level batch_comment_test.go reproduction. Compare the parser's handling of each listed comment style, including multiline blocks, and review conn_batch.go:19-20 for the duplicate patterns. Done means commented-out INSERTs are ignored while the real table and columns are extracted across the affected batch and format paths.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- clickhouse, go
- Domain
- databases
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 52/100