check carefully: plugs memory leak in postgresql backend #4459

Merged
merged 1 commit into from Sep 12, 2016

Projects

None yet

5 participants

@ahupowerdns
Member

eleksir noted that we leak a ton of memory in postgresql. I'm no postgres expert, but this plugs my leak and still appears to function. In other news, do we need a transaction for every query?

@ahupowerdns ahupowerdns eleksir noted that we leak a ton of memory in postgresql. I'm no post…
…gres expert, but this plugs my leak and still appears to function. In other news, do we need a transaction for every query?
903bb49
@ahupowerdns
Member

'You can keep a PGresult object around for as long as you need it; it does not go away when you issue a new command, nor even if you close the connection. To get rid of it, you must call PQclear. Failure to do this will result in memory leaks in your application.' - postgresql docs appear to confirm this is needed.

@cmouse
Contributor
cmouse commented Sep 12, 2016

LGTM.

@ahupowerdns ahupowerdns merged commit 2e8802f into PowerDNS:master Sep 12, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ahupowerdns
Member

I have checked the code, this is the only place where we make this mistake. I do note that the code looks 'leak prone'.

@mschout
mschout commented Sep 27, 2016

+1 This bit us hard as well. We applied the patch and things seem much better now. As someone who has worked with libpq for 20 years, I can tell you that calling PQexec() without calling PQclear() on the result will definitely leak memory. Only difference in the patch we applied locally to what was merged was to use "PGresult *res" instead of "auto res" just to be consistent with the style used elsewhere in that file.

Would love to see this fixed in 4.0.2 as this was a big problem for us since moving to 4.0

@mschout
mschout commented Sep 27, 2016

Just adding some info here as evidence this patch fixes a leak. Without the applied patch, dtrace on PQexec(), PQexecPrepared(), and PQclear() entries:

  PQexecPrepared                                                  702
  PQexec                                                         1404
  PQclear                                                        1408

The total PQclear()'s should be equal to the number of PQexec*()'s.

After applying the patch:

  PQexecPrepared                                                  396
  PQexec                                                          792
  PQclear                                                        1188

Which is exactly as it should be.

@mind04 mind04 referenced this pull request Oct 11, 2016
Merged

Auth 4.0.x backports #4558

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment