-
Notifications
You must be signed in to change notification settings - Fork 852
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
add *sql.DB construction from *pgxpool.Pool #1718
Conversation
There seems to be a great deal of code. I would not have expected a sql.DB <-> pgxpool.Pool adapter to be more than 50 LOC. stdlib already uses |
Agree, started to make this solution as a separate module, that's why I had to duplicate a lot of code. Rewrote to a more compact version. |
e5715ff
to
c95bd95
Compare
c95bd95
to
85fb231
Compare
2ae940f
to
01d33b4
Compare
stdlib/sql.go
Outdated
} else { | ||
var pconn *pgxpool.Conn | ||
|
||
if err = c.BeforeAcquire(ctx, c.pool); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whats the use case for sql.Driver
level BeforeAcquire? Pool already has this callbacks internally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good point, fixed!
@jackc can u review again, please? |
The code seems reasonable. But this approach requires the database/sql pool to be configured with At the very least proper usage should be documented. But if |
|
I did just try this branch in integration with a project using |
@redbaron Huh, not sure how I missed that. |
LGTM |
When can we expect the next version to be tagged / released? |
The answer can be found in #1767:
|
If this it I'd like to see if there would be a way to get those stats (critical for pool tuning) |
Closes #1065