postgresml/pgcat

Memory leak? after unclean connection shutdown

Closed this issue · 5 comments

Describe the bug
pgcat memory usage climbs up monotonically if we repeatedly close connections mid-transaction (Maybe due to a memory leak?)

Screen Shot 2022-05-18 at 2 35 00 PM

To Reproduce
Run the following Ruby code and monitor pgcat RSS

require 'pg'
$stdout.sync = true

def work
    conn = PG::connect("postgres://main_user:@pgcat:5432/main_db")
    conn.async_exec("BEGIN")
    conn.close
rescue Exception => e
    puts "Encountered #{e}"
end


sleep 5
loop { 10.times.map { Thread.new { work } }.map(&:join) }

Repeated conn.async_exec("SELECT 1") or conn.async_exec("BEGIN") followed by conn.async_exec("COMMIT") don't cause the issue (pgcat RSS hovers around 3MB in these cases) whereas with the unclosed transaction example, I was able to see a value of 55MB RSS after 10 minutes running.

Expected behavior
RSS shouldn't grow constantly.

Maybe because we are not calling release in this case
?

levkk commented

Very good find, fix incoming.

levkk commented

Fix merged. Let me know if it re-occurs or something else pops up.

Thanks @levkk

One more question though, I see there are some codepaths where we return early (e.g. https://github.com/levkk/pgcat/blob/main/src/client.rs#L366) , do we also want to release there?

Maybe make the call to release as a part of the Drop implementation for the client.

levkk commented

Good question @drdrsh ! Take a look here, we release on error. The corner case was not releasing on early successful return. Nonetheless, I agree with you, this would be better if we release in the Drop implementation. I'll make that change.

Thanks!

P.S. Thinking some more, we also release when we finish a transaction yet the Client object is still used, so maybe releasing in Drop may not be optimal. That being said I think the leak should be plugged now :)