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: close driver.Connector if it implements an io.Closer #41790

Open
tie opened this issue Oct 5, 2020 · 7 comments
Open

database/sql: close driver.Connector if it implements an io.Closer #41790

tie opened this issue Oct 5, 2020 · 7 comments

Comments

@tie
Copy link
Contributor

@tie tie commented Oct 5, 2020

I propose adding a new sql.DB.Close behavior/side effect: closing driver.Connector if it implements an optional io.Closer interface.

See CL 258360 / #41710

We would like to be able to manage the resources in driver.Connector, e.g. to share the same underlying database handle between multiple connections. This is required for some embedded databases with in-memory backends like SQLite and Genji so that we don’t create a new in-memory store whenever sql.DB decides to ask for a new connection by calling driver.Connector.Connect.

For in-memory storage go-sqlite3 driver docs currently advice manually setting cache=shared DSN parameter, and non-zero max idle connection limit and infinite connection lifetime on sql.DB. (see FAQ: Why I'm getting no such table error?).

It get worse with Genji since both on-disk BoltDB and BadgerDB engines lock the database. That is, connector can’t open the database until the existing handle is closed. With database/sql it means that either user must set connection limit to 1, or we have to share the handle between driver.Conn instances, which also allows us to support concurrent transactions with BadgerDB (see genjidb/genji#210).

That said, we need to release the resources held by driver.Connector (e.g. close file handle, unlock database or free unmanaged memory) when sql.DB is no longer needed.

@gopherbot gopherbot added this to the Proposal milestone Oct 5, 2020
@gopherbot gopherbot added the Proposal label Oct 5, 2020
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Oct 14, 2020
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Oct 14, 2020

@kardianos
Copy link
Contributor

@kardianos kardianos commented Oct 14, 2020

The bar to expand or update the existing database/sql (v1) is fairly high

However, the change is simple and won't interact with anything else. There is one other situation that this would help. If a driver for a database without inline connection cancelation has a background goroutine/connection dedicated to closing connections, this would provide a way to prevent that connection from leaking.

I would say tentatively accept.

@kardianos kardianos moved this from Incoming to Likely Accept in Proposals Oct 14, 2020
@rsc rsc moved this from Likely Accept to Active in Proposals Oct 21, 2020
@rsc
Copy link
Contributor

@rsc rsc commented Oct 21, 2020

@kardianos, we'll take care of updating the proposal project status. It needs to go through some discussion before moving to likely accept. Thanks.

@rsc
Copy link
Contributor

@rsc rsc commented Oct 21, 2020

Adding to minutes.

@rsc
Copy link
Contributor

@rsc rsc commented Oct 28, 2020

Does anyone object to adding this?

@rsc
Copy link
Contributor

@rsc rsc commented Nov 4, 2020

Based on the discussion, this seems like a likely accept.

@rsc rsc moved this from Active to Likely Accept in Proposals Nov 4, 2020
@rsc
Copy link
Contributor

@rsc rsc commented Nov 11, 2020

No change in consensus, so accepted.

@rsc rsc moved this from Likely Accept to Accepted in Proposals Nov 11, 2020
@rsc rsc changed the title proposal: database/sql: close driver.Connector if it implements an io.Closer database/sql: close driver.Connector if it implements an io.Closer Nov 11, 2020
@rsc rsc modified the milestones: Proposal, Backlog Nov 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Accepted
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.