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

database/sql: notify driver connection when returning to pool #22049

Closed
kardianos opened this issue Sep 26, 2017 · 9 comments
Closed

database/sql: notify driver connection when returning to pool #22049

kardianos opened this issue Sep 26, 2017 · 9 comments
Assignees
Milestone

Comments

@kardianos
Copy link
Contributor

@kardianos kardianos commented Sep 26, 2017

Connections are re-used between queries. However connection sessions are state-full and modifying that state may affect the next query. It would be beneficial to have a means of notifying the driver when a connection is being returned to the pool asynchronously to user code to allow driver connections to reset their session.

This would also allow driver connections to notify the pool that their connection state is bad, while still returning the real error to the user.

package driver // imports "database/sql/driver"

// ResetSessioner may be implemented by Conn. When called a driver may choose to reset the
// session associated with the connection.
type ResetSessioner interface {
    // ResetSession is called before a connection is returned to the connection pool, but does not
    // block user code during this process. No queries will be running on this connection when this
    // is called.
    //
    // If the connection is bad it should return driver.ErrBadConn to avoid being returned to the
    // connection pool.
    ResetSession(ctx context.Context) error
}

This would also fix #20807

@kardianos kardianos added the Proposal label Sep 26, 2017
@kardianos kardianos added this to the Go1.10 milestone Sep 26, 2017
@kardianos kardianos self-assigned this Sep 26, 2017
@kardianos
Copy link
Contributor Author

@kardianos kardianos commented Sep 27, 2017

@tgulacsi @ mjibson Any comments on this proposal?

@tgulacsi
Copy link
Contributor

@tgulacsi tgulacsi commented Sep 27, 2017

I don't know any implications regarding Oracle.
I use a session pool (Oracle side), and could return the unused session, but would have to acquire a new one when that connection is reused - which I have no information about.

Where has this requirement appeared?

Maybe @mjibson has some suggestions.

@mjibson
Copy link
Contributor

@mjibson mjibson commented Sep 27, 2017

Yes, I think this is a good proposal. It's kind of scary that we didn't have this before. Maybe session state isn't too harmful in general? In any case I like this.

@kardianos
Copy link
Contributor Author

@kardianos kardianos commented Sep 27, 2017

I'm hearing neutral to positive in my feedback so far (@jackc was more neutral, I'm positive, I take it from @tgulacsi it may not apply to the oracle driver, @mjibson is positive).

I also like how it cleanly handles the case of notifying the pool of bad driver connections.

@tgulacsi This was opened from a MS SQL Server driver issue, but I also have personal interest in it. Most of the drivers I'm aware of link session to connection, so the connection pool is really a session pool logically. This isn't really a problem, until a flaky query leave behind a temp table or something, then blows up the next time it gets run in the same session.

@kardianos
Copy link
Contributor Author

@kardianos kardianos commented Sep 27, 2017

cc @mattn

@mattn
Copy link
Member

@mattn mattn commented Sep 27, 2017

Who call this Return? And this Return is called asynchronosy while driver collect rows (i.e. busy)? Then it should return ErrBusy?

@kardianos
Copy link
Contributor Author

@kardianos kardianos commented Sep 27, 2017

I agree Return is probably a bad name. Maybe ResetSession(...). Good point, in the above proposal I don't have good docs saying when this will be called. I'll fix to be clearer. It won't be called then the driver connection is busy with anything else, is will be called after the user code has finished the query, finished the rows, committed the Tx, or returned the dedicated connection.

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 2, 2017

Change https://golang.org/cl/67630 mentions this issue: database/sql: add driver.ResetSessioner and add pool support

@gopherbot gopherbot closed this in 2620ac3 Oct 24, 2017
@gopherbot
Copy link

@gopherbot gopherbot commented Oct 24, 2017

Change https://golang.org/cl/73033 mentions this issue: database/sql: add driver.ResetSessioner and add pool support

gopherbot pushed a commit that referenced this issue Oct 24, 2017
A single database connection ususally maps to a single session.
A connection pool is logically also a session pool. Most
sessions have a way to reset the session state which is desirable
to prevent one bad query from poisoning another later query with
temp table name conflicts or other persistent session resources.

It also lets drivers provide users with better error messages from
queryies when the underlying transport or query method fails.
Internally the driver connection should now be marked as bad, but
return the actual connection. When ResetSession is called on the
connection it should return driver.ErrBadConn to remove it from
the connection pool. Previously drivers had to choose between
meaningful error messages or poisoning the connection pool.

Lastly update TestPoolExhaustOnCancel from relying on a
WAIT query fixing a flaky timeout issue exposed by this
change.

Fixes #22049
Fixes #20807

Change-Id: I2b5df6d954a38d0ad93bf1922ec16e74c827274c
Reviewed-on: https://go-review.googlesource.com/73033
Run-TryBot: Daniel Theophanes <kardianos@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@golang golang locked and limited conversation to collaborators Oct 24, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.