clickhouse-go icon indicating copy to clipboard operation
clickhouse-go copied to clipboard

QueryContext not supported in prepared statement in SQL driver compatibility

Open srebhan opened this issue 11 months ago • 3 comments

Describe the bug

When executing a prepared query statement, the SQL client returns only Exec method supported in batch mode.

Steps to reproduce

  1. docker run --name clickhouse -p 9000:9000 -e ALLOW_EMPTY_PASSWORD=yes bitnami/clickhouse
  2. wait for container to startup
  3. go run main.go

Expected behaviour

Query is executed and rows are returned without an error.

Code example

package main

import (
	"context"
	"database/sql"
	"fmt"
	"log"

	_ "github.com/ClickHouse/clickhouse-go/v2"
)

func main() {
	db, err := sql.Open("clickhouse", "clickhouse://[email protected]:9000")
	if err != nil {
		log.Fatalf("opening database failed: %v", err)
	}
	defer db.Close()

	ctx := context.Background()
	if err := db.PingContext(ctx); err != nil {
		log.Printf("pinging server failed: %v", err)
	}

	stmt, err := db.PrepareContext(ctx, "SELECT * FROM system.metrics")
	if err != nil {
		log.Fatalf("preparing statement failed: %v", err)
	}

	rows, err := stmt.QueryContext(ctx)
	if err != nil {
		log.Fatalf("executing preparing statement failed: %v", err)
	}
	columns, err := rows.Columns()
	fmt.Println("row columns:", columns)
	fmt.Println("err:", err)
}

Configuration

Environment

  • Client version: 2.18.0
  • Language version: Golang
  • OS: Linux/AMD64
  • Interface: ClickHouse database/sql compatible driver

ClickHouse server

  • ClickHouse Server version: tested with 22.x, 23.8 and latest

srebhan avatar Feb 26 '24 14:02 srebhan

Hi @srebhan

This is sort of the consequence Prepare(Context) in clickhouse-go database/sql compatibility layer is supposed to work with batch INSERTs only.

In general, sql.Stmt is meant to work with prepared statements. There is a deprecation note to the Query function:

Deprecated: Drivers should implement StmtQueryContext instead (or additionally).

I think it makes sense to follow the deprecation note and if possible to use driver instance to query.

jkaflik avatar Feb 26 '24 14:02 jkaflik

@jkaflik not sure I can follow. In my code above I first prepare a statement and then use stmt.QueryContext(). According to https://pkg.go.dev/database/sql#Stmt.QueryContext this is perfectly fine and not deprecated. Am I missing something?

We are bound to Golang's SQL framework and cannot use the native client in Telegraf's inputs.sql plugin as this is supposed to work with different drivers dependent on the user's choice...

Is there any way to use prepared statements with the v2 database/sql driver? Version 1 worked without any issues...

srebhan avatar Feb 26 '24 22:02 srebhan

In my code above I first prepare a statement and then use stmt.QueryContext(). According to https://pkg.go.dev/database/sql#Stmt.QueryContext this is perfectly fine and not deprecated. Am I missing something?

You are right. I looked on a wrong function.

Is there any way to use prepared statements with the v2 database/sql driver?

Yes, since like you mention:

We are bound to Golang's SQL framework and cannot use the native client in Telegraf's inputs.sql plugin as this is supposed to work with different drivers dependent on the user's choice...

It makes sense to be the most compatible. In that case, driver.Stmt implementation can use an underlying connection to perform QueryContext.

I can't promise this can be developed this week on my end. I am also happy to review a PR.

jkaflik avatar Feb 27 '24 06:02 jkaflik