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

Open
#1,950 0 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.