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: add `lastUseTime` or similar to driverConn, add SetConnMaxIdleLefttime to DB #25232

Closed
leaxoy opened this issue May 3, 2018 · 9 comments

Comments

@leaxoy
Copy link

@leaxoy leaxoy commented May 3, 2018

I noticed some db like mysql support idle timeout, which means a conn will closed when timeout. In sql package, only has db.SetConnMaxLefttime and driverConn.createdAt available, so I suggest add lastUseTime to driverConn and SetConnMaxIdleLefttime to DB.

@leaxoy leaxoy changed the title [feature] database/sql add `lastUseTime` or similar to driverConn, add SetConnMaxIdleLefttime to DB proposal: database/sql: add `lastUseTime` or similar to driverConn, add SetConnMaxIdleLefttime to DB May 3, 2018
@gopherbot gopherbot added this to the Proposal milestone May 3, 2018
@gopherbot gopherbot added the Proposal label May 3, 2018
@kardianos
Copy link
Contributor

@kardianos kardianos commented May 3, 2018

I don't fully follow what you are suggesting. However, if a driver connection sets an idle timeout in the DSN or settings then the connection will close on idle. If that connection (still in the pool) attempts to be used, it will return driver.ErrBadConn and the pool will try to pull another one.

Actually, it may not be a bad idea to support a method to signal to the database connection pool that the driver is bad right away rather then retrying and risk getting all bad connections. I'm not sure how that would work of hand.

@leaxoy
Copy link
Author

@leaxoy leaxoy commented May 5, 2018

Thanks for your prompt, I find an another way to keep conns alive by Ping it by time ticker.

@leaxoy
Copy link
Author

@leaxoy leaxoy commented May 5, 2018

Sadly, I also had no idea how to achieve this feature now. But we can talk it in the future

@kardianos
Copy link
Contributor

@kardianos kardianos commented May 7, 2018

I think you are proposing a new database pool setting called "set max idle time" where if the connection is idle for ever the max time spent it is closed and removed from the pool.

Is this correct?

@leaxoy
Copy link
Author

@leaxoy leaxoy commented May 8, 2018

Yeah, thanks for your detailed explanation

@rsc
Copy link
Contributor

@rsc rsc commented Jul 23, 2018

@kardianos, does this proposal sound good to you? OK to accept if so.

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 30, 2018

Change https://golang.org/cl/145758 mentions this issue: database/sql: add SetConnMaxIdleTime

@bradfitz bradfitz added the NeedsFix label Feb 8, 2019
@bradfitz bradfitz changed the title proposal: database/sql: add `lastUseTime` or similar to driverConn, add SetConnMaxIdleLefttime to DB database/sql: add `lastUseTime` or similar to driverConn, add SetConnMaxIdleLefttime to DB Feb 8, 2019
@bradfitz bradfitz modified the milestones: Proposal, Go1.13 Feb 8, 2019
@methane
Copy link
Contributor

@methane methane commented Apr 3, 2019

@leaxoy Why SetMaxLifetime() is not enough?

When idle_timeout is 3600sec, SetMaxLifetime(time.Second * 1800) can avoid idle timeout.

I don't want to add MaxIdleTime, because it makes pool more complicated.

@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@smasher164 smasher164 modified the milestones: Backlog, Go1.14 Oct 11, 2019
@odeke-em
Copy link
Member

@odeke-em odeke-em commented Nov 29, 2019

Thank you for sending CL 145758 @kardianos, but unfortunately we didn't get it in for Go1.14. @methane would you mind commenting on the tagged CL about how this new method won't work for you, or how you can use the previous available methods?

Thank you.

@odeke-em odeke-em modified the milestones: Go1.14, Backlog Nov 29, 2019
@gopherbot gopherbot closed this in 6c61a57 Feb 21, 2020
@ianlancetaylor ianlancetaylor mentioned this issue Jun 12, 2020
133 of 133 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

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