ClickHouse / clickhouse-go

Golang driver for ClickHouse
Apache License 2.0
2.87k stars 550 forks source link

clickhouse.stdDriver implement the sql/driver/ConnBeginTx interface #733

Open albertlockett opened 2 years ago

albertlockett commented 2 years ago

Is your feature request related to a problem? Please describe. I'm trying to use the otelsql instrumentation and running into a problem when I begin a transaction.

// main.go
conn = otelsql.OpenDB(clickhouse.Connector(opts))
tx, err := conn.Begin()

otelsql's driver.Conn implementation does implement ConnBeginTx, so golang will call it's BeginTx method https://github.com/golang/go/blob/master/src/database/sql/ctxutil.go#L98

otelsql expects it's connection to also implement the ConnBeginTx interface, so this will return an err https://github.com/XSAM/otelsql/blob/main/conn.go#L170

Describe the solution you'd like Currently stdDriver only implements driver.Conn https://pkg.go.dev/database/sql/driver#Conn e.g. It exposes a BeginTx method, which is allegedly deprecated:

type Conn interface {
       ...
        // Begin starts and returns a new transaction.
    //
    // Deprecated: Drivers should implement ConnBeginTx instead (or additionally).
    Begin() ([Tx](https://pkg.go.dev/database/sql/driver#Tx), [error](https://pkg.go.dev/builtin#error))

We could add an additional BeginTx method to the struct so it will implement ConnBeginTx https://pkg.go.dev/database/sql/driver#ConnBeginTx

According to the interface definition, we'll need to check the txn options argument and return some errors if ReadOnly and some Isolation levels aren't supported. Here's how the standard library does these checks, so we can do something similar. https://github.com/golang/go/blob/master/src/database/sql/ctxutil.go#L110

i.e. add something like this:

func (std *stdDriver) BeginTx(ctx context.Context, opts TxOptions) (Tx, error) {
    if opts != nil {
        // Check the transaction level. If the transaction level is non-default
        // then return an error here as the BeginTx driver value is not supported.
        if opts.Isolation != LevelDefault {
            return nil, errors.New("sql: driver does not support non-default isolation level")
        }

        // If a read-only transaction is requested return an error as the
        // BeginTx driver value is not supported.
        if opts.ReadOnly {
            return nil, errors.New("sql: driver does not support read-only transactions")
        }
    }

    if ctx.Done() == nil {
        return std.Begin()
    }
}

Describe alternatives you've considered If we don't want to implement this interface, maybe I can get the otelsql to support using the deprecated Begin method

Additional context

std library calling BeginTx on otConn

Screen Shot 2022-08-25 at 2 40 50 PM

otConn returning the err cause stdDriver doesn't implement BeginTx

Screen Shot 2022-08-25 at 1 24 06 PM
XSAM commented 1 year ago

Related https://github.com/XSAM/otelsql/pull/153