-
Notifications
You must be signed in to change notification settings - Fork 629
Description
This is to track the issues/experiments discussed on this issue.
particularly this comment.
Context
It's generally nice to handover query param binding in the query to the server side for multiple reasons.
- No special escape handling on client-side (if error prone, can lead to SQL inject attacks)
- Server can do certain optimizations on it's side
- Less work for clients in general
This is common on even popular database drivers in Go ecosystem. Say in mysql and postgres world where server side binding is default.
Experiment.
(copied from this comment)
Problem
The problem is two folds
- ClickHouse server accepts params via
SET param_value=10; SELECT * FROM test WHERE value={value:Int64}
NOTE: Here, the client need to send both the value and it's type to the server.
- Go client currently accepts three different ways to bind the parameters.
a. Positional binding.
conn.Query("SELECT * FROM test WHERE name=? AND value=?", "some-name", some-value)b. Numeric binding.
conn.Query("SELECT * FROM test WHERE name=$2 AND value=$1", "some-name", some-value)c. ClickHouse native binding
params := clickhouse.WithParameters(clickhouse.Parameters{
"value": "10",
})
ctx := clickhouse.Context(context.Background(), params)
rows, err := conn.QueryContext(ctx, "SELECT * FROM test WHERE value={value: Int64}")(c) is server side binding. The problem is with (a) and (b) which are client side binded now. Notice that arguments passed for both positional binding and numeric binding are of type any. Meaning we cannot infer exact type (int vs int32 vs int64 vs float32 vs float64, etc). Basically in order to convert those into server side binding, we need to know their exact type to send it to ClickHouse server. Which we cannot.
(a) and (b) is the standard way Go's sql.DB works unfortunately.
(@mshustov probably this is the one you were saying that Go's standard is the blocker to migrate to server-side binding)
Proposal
- We can deprecate using both positional and numeric binding in our Go client and warn the user to switch to clickhouse native binding
- Properly support both client-side and server-side binding.
Both has tradeoffs. Neither is is the single best way to solve the problem. I'm bit concerned with (1) because don't know how users (both internal and OSS) would consider this change, given this is the standard way how people using SQL in Go.
Open for any thoughts and ideas
Current consensus
I'm also tying to change the api a little bit so that it is possible to move to server-side binding even for positional and numberic bindings. But still WIP.