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

database/sql: don't encumber Context with options #18284

Closed
tgulacsi opened this Issue Dec 11, 2016 · 12 comments

Comments

Projects
None yet
7 participants
@tgulacsi

tgulacsi commented Dec 11, 2016

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

go1.8beta1

What operating system and processor architecture are you using (go env)?

linux/amd64

What did you do?

See https://groups.google.com/forum/#!topic/golang-nuts/20NtIGTgBeg

What did you expect to see?

A nice, clean API just as before, and not using context.Context for transferring options.

What did you see instead?

ReadOnlyContext and IsolationContext to passes isolation level and readonly flag in the Context.

I'd modify BeginContext as

func (db *DB) BeginContext(ctx context.Context, options... TxOption) (*Tx, error)

type TxOption func(*txOption)

type txOption struct {
    IsReadOnly bool
    IsolationLevel
}

func WithIsolationLevel(level IsolationLevel) TxOption { return func(o *txOption) { o.IsolationLevel = level } }
func WithReadOnly(readOnly bool) TxOption { return func(o *txOption) { o.IsReadOnly = readOnly } }

This does not block anybody to use the Context as bag-of-values, but at least does not encourage that.

@eandre

This comment has been minimized.

eandre commented Dec 11, 2016

I think in order for this to be considered we need a proposal that also encompasses how to handle options for the other methods that also take a context today (QueryContext, QueryRowContext, ExecContext, PrepareContext).

@tgulacsi

This comment has been minimized.

tgulacsi commented Dec 11, 2016

But those (AFAIK) does not use ReadOnly/IsolationLevel!

And I don't have a good solution for passing option in the other context functions...

@kardianos

This comment has been minimized.

Contributor

kardianos commented Dec 11, 2016

I don't see how the QueryContext, QueryRowContext, And ExecContext could be chagned to take an options. If the pattern was to make a sql.Cmd and pass that in, we would have room for that. As it stands I don't think we should change those signatures. I was mainly relying to a previous question when I mentioned that.

The only way I could see is if we had a strong use case to pass query specific options maybe we could allow a specific Option type to the argument list at a future date.

For BeginContext I am personally fine with sql.BeginContext(context.Context) where the context takes transaction start options. I will grant that may not be entirely idiomatic.

For the sake of exploration because go1.8 has not entirely shipped yet (though the timing is very late), maybe we could do the following:

package sql

func (db *DB) BeginContext(ctx context.Context, opts ...driver.TxOption) (*Tx, error)

func WithIsolation(level IsolationLevel) driver.TxOption {
   return func() (key, value interface{}) {
        return internal.IsolationLevelKey, level
   }
}

func WithReadOnly() driver.TxOption {
   return func() (key, value interface{}) {
        return internal.ReadOnlyKey, true
   }
}

package driver

type TxOption func() (key, value interface{})

This would allow drivers to define custom attributes for transactions. In the original issue that was one of the sticking points. At this point we have just re-created the context value setting.

Again, I'm not thrilled with a change this late, but let me know what you think of that counter proposal, I'll also cc @bradfitz to see what he says about the API / API changes at this point in time.

@balasanjay

This comment has been minimized.

Contributor

balasanjay commented Dec 12, 2016

I think context is designed to be propagated downward (possibly as child contexts).

It seems unexpected to me as a library-author that clients of my library can modify the isolation level in my db accesses without me ever mentioning the phrase "IsolationLevel" in my code. The context design seems to inadvertently expose API to transitive callers, rather than just to immediate callers.

@kardianos

This comment has been minimized.

Contributor

kardianos commented Dec 12, 2016

@balasanjay If you don't want to affect downstream then don't pass on the ctx you use to set the iso level. I don't see this as a pragmatic issue (I could be wrong), but I do agree that this is less than idiomatic. I could envision using it either at the call site or like:

switch {
case strings.HasPrefix(r.URL.Path, "/api/"):
    ctx, _ = context.WithTimeout(ctx, time.Second * 3)
    ctx = sql.WithIsolation(sql.LevelReadCommitted)
case strings.HasPrefix(r.URL.Path, "/report/"):
    ctx, _ = context.WithTimeout(ctx, time.Second * 60)
    ctx = sql.WithReadOnly(ctx)
}

Maybe that would be a bad idea and should be an anti-pattern. But I think it might be just fine.

I'm mainly looking for concrete alternative API designs and technical acceptance or critique of any suggested. It is important to note that this was previously discussed publicly on the issue tracker: #15123 (comment)

I don't know if it is too late to make such a change. But it might be possible if there is a compelling complete alternative that is superior. Please read the linked issue, and comment on any full proposals stated here.

@tgulacsi

This comment has been minimized.

tgulacsi commented Dec 12, 2016

I do like the func (db *DB) BeginContext(ctx context.Context, flags ...TxFlag) (*Tx, error) interface, but absolutely can accept pushing those flags into the Context. Just wanted to probe this before set this in stone (release of Go 1.8).

I'd like to have @bradfitz suggest something regarding this, as he has designed more APIs than all of us...

@bradfitz bradfitz added this to the Go1.8 milestone Dec 12, 2016

@bradfitz bradfitz self-assigned this Dec 12, 2016

@kardianos

This comment has been minimized.

Contributor

kardianos commented Dec 12, 2016

Potential API change:

package sql

type TxOpt struct {
    Level IsolationLevel
    ReadOnly bool
}

// opt is optional and may be null.
func (db *DB) BeginTx(ctx context.Context, opt  *TxOpt) {...}

package driver

// TxOpt copied to from sql.TxOpt.
type TxOpt struct {
    Level IsolationLevel
    ReadOnly
}

type ConnBeginContext interface {
    BeginContext(ctx context.Context, opt *TxOpt) (Tx, error)
}

Feedback requested from interested parties.

@bradfitz

This comment has been minimized.

Member

bradfitz commented Dec 12, 2016

Yup, SGTM.

(Background: Russ, Ian and I chatted about this at the Go 1.8 release meeting, and then I chatted with @kardianos about it after.)

@asifjalil

This comment has been minimized.

asifjalil commented Dec 12, 2016

Quick question. Why are we defining TxOpt twice: once in the sql and another time in the driver package?

@kardianos

This comment has been minimized.

Contributor

kardianos commented Dec 12, 2016

package driver must not reference package sql. User needs to ref struct in sql, driver needs it in driver.

This is one of these cases where I'd like the alias feature. But for now we have duplicate definitions.

@asifjalil

This comment has been minimized.

asifjalil commented Dec 12, 2016

That makes sense. Thanks @kardianos.

@gopherbot

This comment has been minimized.

gopherbot commented Dec 13, 2016

CL https://golang.org/cl/34330 mentions this issue.

@gopherbot gopherbot closed this in d0501f1 Dec 14, 2016

@golang golang locked and limited conversation to collaborators Dec 14, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.