ClickHouse / clickhouse-go

Golang driver for ClickHouse
Apache License 2.0
2.88k stars 553 forks source link

Always compress responses when the client compression is on #1286

Closed zhkvia closed 4 months ago

zhkvia commented 5 months ago

Summary

CLAassistant commented 5 months ago

CLA assistant check
All committers have signed the CLA.

zhkvia commented 5 months ago

Actually after applying my fix and running it for some time in production I noticed that errors returned from a failed DESCRIBE don't get decompressed:

clickhouse [execute]:: 404 code: ~k��"x�
�h��`������Code: 60. DB::Exception: Table logs.�_shard does not exist. Maybe you meant0�0_trace?. (UNKNOWN_TABLE) (version 23.8.12.13 (official build))

So I looked at the source code more closely and noticed the difference between DESCRIBE and INSERT queries:

  1. responses from DESCRIBE queries are always compressed:
    1. https://github.com/ClickHouse/clickhouse-go/blob/main/conn_http_batch.go#L54
    2. https://github.com/ClickHouse/clickhouse-go/blob/main/conn_http_query.go#L39
  2. while responses from INSERT queries are not:
    1. https://github.com/ClickHouse/clickhouse-go/blob/main/conn_http_batch.go#L52
    2. https://github.com/ClickHouse/clickhouse-go/blob/main/conn_http_batch.go#L108
    3. https://github.com/ClickHouse/clickhouse-go/blob/main/conn_http_batch.go#L201

But responses are always decompressed no matter what a query it was: https://github.com/ClickHouse/clickhouse-go/blob/main/conn_http.go#L432

So in order to fix this, I explicitly set compress option for INSERT queries as well


More than that, I noticed that in older versions of the library EOF was treated explicitly because it was wrapped with other errors several times in a row (e.g. read next block: header: EOF): https://github.com/ClickHouse/clickhouse-go/blob/v2.14.0/conn_http.go#L453

But now this error is not checked and ReadAll can't handle it by itself since it doesn't look inside wrapped errors: https://github.com/ClickHouse/clickhouse-go/blob/main/conn_http.go#L436

So I fixed this too

zhkvia commented 5 months ago
run-tests / single-node (1.21, 24.1) (pull_request) Failing after 1m

Well, I couldn't reproduce the same results locally:

 λ  CLICKHOUSE_VERSION=24.1 go test .
ok      github.com/ClickHouse/clickhouse-go/v2  0.392s
 λ  cd tests/issues
 λ  CLICKHOUSE_VERSION=24.1 go test .
ok      github.com/ClickHouse/clickhouse-go/v2/tests/issues     0.565s
 λ  go version
go version go1.21.9 darwin/amd64

Also

 λ  CLICKHOUSE_VERSION=24.1 make test

runs fine