Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
albertlockett opened this issue Aug 25, 2022 · 1 comment
Open

Comments

@albertlockett
Copy link
Contributor

albertlockett commented Aug 25, 2022

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
Copy link

XSAM commented Mar 9, 2023

Related XSAM/otelsql#153

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants