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

fix: memory leak in database/sql integration #162

Merged
merged 3 commits into from
Apr 4, 2022

Conversation

stepan-romankov
Copy link
Contributor

Change Description

There is a memory leak in postgres/pgxv4/postgres.go:60 func (p *pgDriver) Open(name string) (driver.Conn, error)
This happens because stdlib.RegisterConnConfig() on every connection opening returns new dbURI and stores new instance of pgx.ConnConfig to map indexed by dbURI

Relevant issues:

@stepan-romankov stepan-romankov requested a review from a team April 4, 2022 11:17
@google-cla
Copy link

google-cla bot commented Apr 4, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

For more information, open the CLA check for this pull request.

@kurtisvg
Copy link
Contributor

kurtisvg commented Apr 4, 2022

Hi @stepan-romankov - Thanks for taking the time to open an issue, identify the cause, and suggest a fix. It's greatly appreciated.

Are you able to sign the CLA? We need to have the CLA signed before we can leave feedback on your PR or accept the change.

Copy link
Member

@enocom enocom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for sending a fix for this!

@stepan-romankov
Copy link
Contributor Author

Hi @stepan-romankov - Thanks for taking the time to open an issue, identify the cause, and suggest a fix. It's greatly appreciated.

Are you able to sign the CLA? We need to have the CLA signed before we can leave feedback on your PR or accept the change.

Should already be done...

@kurtisvg
Copy link
Contributor

kurtisvg commented Apr 4, 2022

Should already be done...

Unfortunately, I just ran the CLA check and it's still showing up missing. You can follow the instructions in that link to check your status.

@stepan-romankov
Copy link
Contributor Author

Looks ok now.

@stepan-romankov
Copy link
Contributor Author

stepan-romankov commented Apr 4, 2022

Thanks for sending a fix for this!

I implemented all the requested changes. Please review @enocom.

Copy link
Member

@enocom enocom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two small things. Otherwise LGTM

@stepan-romankov
Copy link
Contributor Author

@enocom done.

@enocom enocom self-requested a review April 4, 2022 20:53
Copy link
Member

@enocom enocom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Memory leak in Postgres database/sql integration
3 participants