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 postgresql connection leak #2611

Conversation

james-lawrence
Copy link
Contributor

@james-lawrence james-lawrence commented Mar 31, 2017

fixes #2410

the connection leak caused by how pgx manages connection pools and how telegraf connects to generate the sql.DB connection. We needed to close the pool in addition to the db connection.

I've patched pgx to handle both DSN and URI based connection strings natively. this fixes the issue.

@james-lawrence
Copy link
Contributor Author

My changes to pgx to avoid this issue entirely have been given the okay. Once they are merged I will update this PR.

@james-lawrence james-lawrence force-pushed the postgresql/fix-connection-leak branch from ddaedbe to 1a5cc39 Compare April 2, 2017 11:09
@james-lawrence
Copy link
Contributor Author

updated PR using fix in the driver.

@@ -79,21 +75,3 @@ func ParseURL(uri string) (string, error) {
sort.Strings(kvs) // Makes testing easier (not a performance concern)
return strings.Join(kvs, " "), nil
}

func Connect(address string) (*sql.DB, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is used by the postgresql_extensible input, can you flip through that plugin and make sure its working and connecting in the correct way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, I added the connect method originally. I'm just fixing the regression for the code I commited. You'll notice I changed postgresql_extensible as well in the diff.

Thanks for the review. =)

@danielnelson danielnelson added this to the 1.3.0 milestone Apr 3, 2017
@danielnelson danielnelson merged commit 5ffc9fd into influxdata:master Apr 5, 2017
@james-lawrence james-lawrence deleted the postgresql/fix-connection-leak branch April 5, 2017 00:39
calerogers pushed a commit to calerogers/telegraf that referenced this pull request Apr 5, 2017
@anilkulkarni87
Copy link

anilkulkarni87 commented Apr 11, 2017

Can we connect to Redshift using this plugin?

@danielnelson
Copy link
Contributor

@anilkulkarni87 I think so, but I have never tried, you could ask over on the InfluxData Community site.

@anilkulkarni87
Copy link

@danielnelson I have been trying to connect. but doesnt connect to Redshift though. Let me tryasking in the community.

@james-lawrence
Copy link
Contributor Author

@anilkulkarni87 its not intended to. so short answer is no =)

@anilkulkarni87
Copy link

anilkulkarni87 commented Apr 11, 2017

@james-lawrence Thought so after going through the .Go script. But do you know if there are any plugins which can connect to redshift?

@james-lawrence
Copy link
Contributor Author

no idea, doesn't look like. You'll likely have to write one.

@danielnelson
Copy link
Contributor

@anilkulkarni87 You can create a feature request issue, make sure to include what happens if you try to use the existing postgres inputs.

@anilkulkarni87
Copy link

@danielnelson created a feature request issue
#2683

vlamug pushed a commit to vlamug/telegraf that referenced this pull request May 30, 2017
maxunt pushed a commit that referenced this pull request Jun 26, 2018
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.

Postresql plugin does not close connections properly
3 participants