pgx.Conn: Fix memory leak: Delete items from preparedStatements #1614
Conversation
|
I could not find a way to test this using the public interface, so I wrote a test that directly inspects the internals of this type. I guess the alternative might be to check the amount of memory being used? This seemed like the easiest way to do this to me, but I would be happy to take better ideas! |
Previously, items were never removed from the preparedStatements map. This means workloads that send a large number of unique queries could run out of memory. Delete items from the map when sending the deallocate command to Postgres. Add a test to verify this works. Fixes #1456
|
LGTM - good catch and fix! |
| @@ -1326,6 +1326,7 @@ func (c *Conn) deallocateInvalidatedCachedStatements(ctx context.Context) error | |||
|
|
|||
| for _, sd := range invalidatedStatements { | |||
| pipeline.SendDeallocate(sd.Name) | |||
| delete(c.preparedStatements, sd.Name) | |||
There was a problem hiding this comment.
this actually might not solve the "leak" issue, since Go won't deallocate memory after you delete a key from a map golang/go#20135
There was a problem hiding this comment.
The map itself won't shrink, but delete() does zero the keys and values in the map, so the *pgconn.StatementDescription value will be garbage collected. Additionally, delete() decrements the size of the map, which means its capacity will now be limited to the max number of prepared statements (with extra space for the map "fill factor"). This means this change should mean the amount of memory used by the c.preparedStatements is now fixed, rather than increasing without bound.
Additionally, my reproduction program on the original issue can run forever without running out of memory: #1456 (comment)
So: I'm pretty confident that this fix will resolve this specific bug at least. There could be others though! I would be happy to try to figure them out if we have a sample program that reproduces the problem.
There was a problem hiding this comment.
delete() does zero the keys and values in the map
yes, only if they poiners
Additionally, delete() decrements the size of the map
I can be wrong, but regarding this example the allocated size of the map would be the same.
which means its capacity will now be limited to the max number of prepared statements
well I guess it's true if the pgx has limitations to prepared statement cache
Previously, items were never removed from the preparedStatements map. This means workloads that send a large number of unique queries could run out of memory. Delete items from the map when sending the deallocate command to Postgres. Add a test to verify this works.
Fixes #1456