Please Review Changes and Fixes to database/sql

[Daniel Theophanes]

All,

When you have time, please review the following database/sql CLs:

https://go-review.googlesource.com/c/go/+/174122

https://go-review.googlesource.com/c/go/+/216197

https://go-review.googlesource.com/c/go/+/216240

https://go-review.googlesource.com/c/go/+/145758

https://go-review.googlesource.com/c/go/+/216241

After some review, I may request some of these make it into a Go1.14 point release if the Go team is amenable to that. But generally we are targeting Go1.15 for these changes.

One API / behavior change is to remove the async connection resetter, and break that into two calls to the driver: the existing SessionResetter, and a new call called "ConnectionDiscarder". The SessionResetter gets called before a new call is made, so it can use the calls context. The ConnectionDiscarder may be called after the connection is finished, but before it enters the connection pool.

Two other bugs have been fixed in this series.

I would like feedback on the addition of SetConnMaxIdleTime (CL 145758). Please leave feedback on the issue https://github.com/golang/go/issues/25232  (unless it is about the implementation, then reply to the CL).

-Daniel

[Tamás Gulácsi]

Nice fixes, thanks!

Is that new "ConnectionDiscarder" is called "Validator" on master?

So, SessionResetter is called before Validator, which is called right before inserting the free connection into the pool?

I'm asking this because the Oracle guys nagging me for some kind of pool disabling possibility,

because their session pool is better...

Which is somewhat true (it knows a lot of Oracle-specific thing, such as failover and tagging, and so on).

One possibility is to set db.SetMaxIdleConns(0), but that needs a cooperating user.

That Validator sounds just what we need, except we should call the SessionResetter right when the connection is pulled from the pool.

The best would be a hook that's get called when the connection is put into the pool,

and another one when its get from the pool.

That way the underlying session could be put into the Oracle session pool and get back simultaneously with the Go pool.

Any ideas?

Thanks,

Tamás Gulácsi

[Daniel Theophanes]

Yes, The ConnectionDiscarder is now Validator.

The Validator.IsValid is called right before the connection is placed into the connection pool.

The ResetConnection is called right before a connection is given out from the connection pool.

The background resetter routine has removed.

Those may work for your purposes.

On a side note, in a Go database/sql/v2 I would imagine the connection pool, prepared statement pool, and the value decoder to be interfaces that can be swapped out on the back end. Then on the front end (ORMs, etc) we would provide a default experience, but then have a more flexible decoding routine that would allow different decoding strategies to efficiently operate.

It sounds like a completely swappable connection pool implementation would be useful for Oracle (and DB2 and maybe others). 

-Daniel

--
You received this message because you are subscribed to the Google Groups "golang-sql" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-sql+...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/golang-sql/8823a416-52d0-4e46-9324-40636adacceb%40googlegroups.com.

[Anthony Tuininga]

Thanks for the explanation. I am one of the people that have been asking Tamás these questions. :-) To be clear, the Go pooling mechanism prevents the necessity of building up the Go connection structure repeatedly and this is a good thing, I think. The need is merely to have a hook when the connection is put into the pool (after doing its work) and just before it is taken out of the pool (about to do its work). If that is in place there would be no need to "replace" the Go pooling mechanism at all.

It sounds like Validator(), which is called right before the connection is placed into the connection pool, and ResetConnection(), which is called right before a connection is given out from the connection pool, is just what we need. This is assuming that the connection isn't put into the pool until after it is has finished its work and is about to become idle (and therefore usable by other calls to db.Exec(), db.Query(), etc.)

Anthony

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

[Daniel Theophanes]

Other then possibly a Conn.Close() (due to expired connections, or similar), no queries would be executed between IsValid and ResetConnection(). However, these won't always be called now that I'm thinking about it more. Oh, wait, I think there might be a subtle bug in current master where the connection isn't always reset in a certain condition in the pool. Also, it may be possible when the the pool is under load that "IsValid" won't be called, but ResetConnection will be (where a returned connection is given directly to a new waiting query (this is by design and not a but).

I need to write the design up and document for drivers more clearly and the Go1.15 release notes. And fix that bug.

...

Also, not thinking of having each driver implement its own connection pool, but merely make the concerns separate and more testable, and in extreme cases, replaceable or wrappable.

-Daniel

To unsubscribe from this group and stop receiving emails from it, send an email to golang-sql+...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/golang-sql/00927b69-14a9-4425-ac38-6ef8757ee9e8%40googlegroups.com.

[Anthony Tuininga]

That all sounds workable. Thanks.

It also sounds like you have your work cut out for you! :-)

[Christopher Jones]

On 25/4/20 1:41 am, Daniel Theophanes wrote:

Other then possibly a Conn.Close() (due to expired connections, or similar), no queries would be executed between IsValid and ResetConnection(). However, these won't always be called now that I'm thinking about it more. Oh, wait, I think there might be a subtle bug in current master where the connection isn't always reset in a certain condition in the pool. Also, it may be possible when the the pool is under load that "IsValid" won't be called, but ResetConnection will be (where a returned connection is given directly to a new waiting query (this is by design and not a but).

To be useful for our purpose, the hooks need to be called consistently so the Oracle libraries can keep in sync with the database/sql pool and know when connections are in use in the application, and when they are idle in the pool.  We want to be able to do Oracle-level connection validity checks, reset user state, and enable & disable the necessary state for high availability features, etc.

A completely swappable implementation in database/sql/v2 would be ideal, though the above hooks would allow us to do any state changes on connections and not even need Oracle's "session pool" under the covers.

What is the design reason isValid isn't called in all cases?

Chris

-- 
https://twitter.com/ghrd

[Daniel Theophanes]

IsValid won't be called if the connection doesn't pass through the connection pool, but is directly passed from an returning connection to a new query.

In this case, ResetSession is only called. If for some reason isn't valid, it can also return ErrBadConn.

[Christopher Jones]

On 25/4/20 11:42 am, Daniel Theophanes wrote:

IsValid won't be called if the connection doesn't pass through the connection pool, but is directly passed from an returning connection to a new query.

In this case, ResetSession is only called. If for some reason isn't valid, it can also return ErrBadConn.

Thank you.

Chris

-- 
https://twitter.com/ghrd

[Daniel Theophanes]

I inspected the sql code tonight. What I thought was a bug, turned out to be correct. So yay.

So what is in master is what I expect to be in 1.15. I'll look into documentation and release notes.

[Daniel Theophanes]

I've sent a CL enhancing the database/sql/driver package docs that I hope will provide a useful overview of the package:

https://go-review.googlesource.com/c/go/+/230214

I'd love some eyes on it if someone has a minute. Feel free to respond on the change (preferred) or even here.

Thanks.

[jack]

LGTM. Actually answers a question about custom types I ran into a couple days ago.