Isn't the use of Context in db/sql non-diomatic?

[Chandru]

Documentation of the context package says,

"Use context Values only for request-scoped data that transits processes and APIs, not for passing optional parameters to functions."

sql.BeginContext introduced in 1.8, uses Context to receive options like IsolationLevel and read-only flag. These are neither request-specific nor cross-cutting. They are options that are typically specific to a type of operation, but common to all requests.

Isn't this use in db/sql contradicting the recommendation in context's doc?

--
Chandra Sekar.S

[parais...@gmail.com]

Either the doc should be changed or the std lib should follow the spirit of the doc. But my understanding is that context.Context is not just about HTTP request/response cycle but deals with anything that needs cancellation or a resource clean up signal , some kind of DIY RAII . In any case the documentation should be clarified. 

[Chandru]

I can understand db/sql using Context for cancellation. It is the optional arguments to BeginContext like IsolationLevel and read-only flag, which are not request-specific, that seem to contradict context's documentation.

--
Chandra Sekar.S

--
You received this message because you are subscribed to the Google Groups "golang-nuts" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Chandru]

Bump

--
Chandra Sekar.S

[Daniel Theophanes]

Hi Chandra,

In my view using context to store value for these uses (Read-Only and Isolation Level) is somewhat of a gray area. They are not a classical request only scope, like a tracing ID, but they also aren't strictly limited to a single request either. Say you wanted to kick off a request, but ensure you don't modify the database in anyway. you could pass in a context with ReadOnly set and any further request function will follow that directive. Similarly, by allowing you to set the Isolation at a higher level, you can effectively influence the behavior of the queries for any subsequent request action.

Should we do it this way? Maybe, maybe not. I think there are benefits to this approach, and I think they can be considered request scoped, even if in the degenerate case they are set at the query level. However, I think it will be really common to set these when dispatching requests in the router and using them for the subsequent requests.

Has /report/ prefix? Set to ReadOnly and Isolation X

Has /api/ prefix? Set Isolation to Y

At least that is how I would envision using them. I have been known to be wrong before.

What do you think? -Daniel

Bump

--
Chandra Sekar.S

--
Chandra Sekar.S

To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts...@googlegroups.com.

[Dave Cheney]

I agree with this. This feels like a case of abusing the "bag of values" nature of context's Value() feature to smuggle arbitrary and unstructured data in and out of an interface rather than change the API.

[André Eriksson]

Daniel, I would characterize the use case you describe as a very indirect way of setting these options on transactions. That's can be a good thing when contexts are already passed around, and coming up with new ways of passing such options down through a few layers of code is some amount of work. It's certainly convenient that they are part of the context in that case.

That said, I believe there are large downsides to this indirect API design. Even when you benefit from this convenience, it encourages a form of "spooky action at a distance" programming where it's not clear at the BeginContext call site what the properties of the transaction will be. Using Go has made me very appreciative of other language features that directly remove this "spooky action at a distance" from other programming languages. Not having exceptions is a great example of this. I would say that Go trades convenience in favor of clarity in those circumstances, and I'm concerned that we're making the opposite tradeoff here.

One can then easily argue that nothing is stopping me or anybody else from forgoing the indirect nature and finding a way of propagating these options down to the code that actually calls BeginContext. In fact, that's probably what I would do. The problem then is that the API is needlessly awkward; it would be much more straightforward to expose an API with functional options if that's the encouraged way of using it.

Another worry of mine is that contexts are passed on to other functions. Even though it's rare that a single request consists of multiple transactions, it still happens and I wouldn't be too surprised to find people accidentally setting the wrong transaction properties simply because it was accidentally inherited from the context that was used to begin another transaction. Again, being more explicit in the API design would cause people to stop and think before passing along a set of transaction options from an unrelated transaction, similar to how Go's explicit error handling forces you to think about errors, even if it's less convenient.

To end with a concrete proposal, I think changing the signature to be "func (*DB) BeginContext(ctx context.Context, opts ...TxOption)" or similar (and then changing the transaction properties to be TxOptions) would solve most of the issues I highlighted above.

André

[Daniel Theophanes]

André,

Thanks for the constructive feedback. I agree the signature you propose would be another way to do this.

For people who care about these settings, I would expect multiple queries per request is normal and ensuring the props are the same sounds like a benefit, not a detriment. In other words, I think you may be underestimating how tied these are to a request.

As another way of framing this, what do people have in their context Value whitelist?

[André Eriksson]

Daniel,

I agree that multiple queries are commonplace. I was referring to multiple transactions within a single request to be rare, and when it happens you don't necessarily want to share the same (read-only, isolation level) properties.

You do have a point that there is a use case for making multiple queries, without using a transaction, that you likely would want to share the same properties. However, my understanding is that isolation level is a meaningless concept without a transaction, and that leaves only read-only as the property of concern (for now?). It's not clear from the documentation whether the read-only property actually affects the other *sql.DB methods that take contexts; it only refers to BeginContext.

I also don't think it's unwarranted to argue that if you want to conveniently ensure that multiple queries share the same properties that you can do so with a transaction. I understand that in rare circumstances the transaction would come with other, undesirable SQL semantics, but in those cases you might as well go through the trouble of dealing with this yourself. Either by passing the options to each query yourself, or to create some application-specific abstraction that handlesit for you.

[Tamás Gulácsi]

I'd also like to change the API for passing options as Andrè suggested: by optional funcs. But everywhere - they're not to be in a Context!

To put them in Context feels a hack.

[Daniel Theophanes]

How would you put them in query when args are the last ...warm?

On Sat, Dec 10, 2016, 22:44 Tamás Gulácsi <tgula...@gmail.com> wrote:

I'd also like to change the API for passing options as Andrè suggested: by optional funcs. But everywhere - they're not to be in a Context!

To put them in Context feels a hack.

--
You received this message because you are subscribed to a topic in the Google Groups "golang-nuts" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/golang-nuts/20NtIGTgBeg/unsubscribe.
To unsubscribe from this group and all its topics, send an email to golang-nuts...@googlegroups.com.

[Gulácsi Tamás]

Good question.

QueryContext(ctx context.Context, query QueryWithOpts, args ...interface{}) (sql.Rows, error)
?

Where 

type QueryWithOpts struct {

    fmt.Stringer
    opts []qryOpt 
}

and

func Query(qry string, options ...qryOpt) QueryWithOpts

á'lá sql.NamedArg.

But this feels just as hacky, as putting those options into the context...

[Daniel Theophanes]

I don't think ops in context is how I would clean room design this, but it is the better of many other options.

The tx begin is easier, but still difficult when I need to ensure drivers can set custom attributes. The problem with something like you suggested is it deviates too far from the original API.

If you have a concrete proposal, file an issue cc me in the next day or two. At the least tx begin needs a way for drivers to set custom attributes.

[Tamás Gulácsi]

2016. december 11., vasárnap 16:20:51 UTC+1 időpontban Daniel Theophanes a következőt írta:

I don't think ops in context is how I would clean room design this, but it is the better of many other options.

The tx begin is easier, but still difficult when I need to ensure drivers can set custom attributes. The problem with something like you suggested is it deviates too far from the original API.

If you have a concrete proposal, file an issue cc me in the next day or two. At the least tx begin needs a way for drivers to set custom attributes.

https://github.com/golang/go/issues/18284

Should I send a CL? We could discuss it in the issue. 

[Chris Hines]

I agree with the sentiment that putting these options in context.Context values is inappropriate.

Chris