Fix leaking of memory and memory contexts in Foreign Constraint Graphs #7236
Conversation
Codecov Report
@@ Coverage Diff @@
## main #7236 +/- ##
==========================================
- Coverage 93.23% 93.23% -0.01%
==========================================
Files 275 275
Lines 59484 59488 +4
==========================================
+ Hits 55462 55464 +2
- Misses 4022 4024 +2 |
|
marking as relevant: https://github.com/citusdata/citus/pull/6234/files Probably doesn't resolve the issue, but at least invalidation becomes less spread over the code |
| fConstraintRelationshipGraph = (ForeignConstraintRelationshipGraph *) palloc( | ||
| sizeof(ForeignConstraintRelationshipGraph)); |
There was a problem hiding this comment.
Resetting ForeignConstraintRelationshipMemoryContext would invalidate fConstraintRelationshipGraph as well and then the next time IsForeignConstraintRelationshipGraphValid would be accessing a freed memory?
There was a problem hiding this comment.
https://github.com/citusdata/citus/pull/7236/files#diff-e0be0392562ddabab1d64b436a7cd8225ca5678d64b80c6f01c2be615a3848a8R327 Should have set that to NULL already no? I could add an assert to verify and make that case clear I guess
There was a problem hiding this comment.
Should have set that to NULL already no?
Right. And when looking into that function, I realized that hash_destroy() already deletes the memory context. I know it's more efficient to reset a memory context rather than deleting it but wondering how we were leaking the memory before then.
There was a problem hiding this comment.
When creating a hashmap with the HASH_CONTEXT flag, like we do, the hashmap has an internal context as a child of its current context:
https://github.com/postgres/postgres/blob/ffb69b23115a1ca1d81f7e273d50b75154ae7b0b/src/backend/utils/hash/dynahash.c#L380-L387
This context is then stored on the hashmap via
https://github.com/postgres/postgres/blob/ffb69b23115a1ca1d81f7e273d50b75154ae7b0b/src/backend/utils/hash/dynahash.c#L501-L505
Which is then destroyed when hash_destroy deletes its context. Hashmaps would be very hard to work with if they start destroying memory contexts that were being used when the map gets created ;)
The context we create is the one that is leaking.
I think that makes sense, I will double check the behaviour of memcontext reset, but I assume it would delete all child contexts as well. |
thanodnl
force-pushed
the
fix/foreign-key-memory-context-leak
branch
from
7346f93
to
dc7ddf8
Compare
Previously, every time we (re)created the Foreign Constraint Relationship Graph, we created a new Memory Context while loosing a reference to the previous context. This old context could still have left over memory in there causing a memory leak. With this patch we statically have one memory context that we lazily initialize the first time we create our foreign constraint relationship graph. On every subsequent creation, beside destroying our previous hashmap we also reset our memory context to remove any left over references.
…nd create if not already created
thanodnl
force-pushed
the
fix/foreign-key-memory-context-leak
branch
from
dc7ddf8
to
d33ecd0
Compare
#7236) DESCRIPTION: Fix leaking of memory and memory contexts in Foreign Constraint Graphs Previously, every time we (re)created the Foreign Constraint Relationship Graph, we created a new Memory Context while loosing a reference to the previous context. This old context could still have left over memory in there causing a memory leak. With this patch we statically have one memory context that we lazily initialize the first time we create our foreign constraint relationship graph. On every subsequent creation, beside destroying our previous hashmap we also reset our memory context to remove any left over references.
#7236) DESCRIPTION: Fix leaking of memory and memory contexts in Foreign Constraint Graphs Previously, every time we (re)created the Foreign Constraint Relationship Graph, we created a new Memory Context while loosing a reference to the previous context. This old context could still have left over memory in there causing a memory leak. With this patch we statically have one memory context that we lazily initialize the first time we create our foreign constraint relationship graph. On every subsequent creation, beside destroying our previous hashmap we also reset our memory context to remove any left over references.
#7236) DESCRIPTION: Fix leaking of memory and memory contexts in Foreign Constraint Graphs Previously, every time we (re)created the Foreign Constraint Relationship Graph, we created a new Memory Context while loosing a reference to the previous context. This old context could still have left over memory in there causing a memory leak. With this patch we statically have one memory context that we lazily initialize the first time we create our foreign constraint relationship graph. On every subsequent creation, beside destroying our previous hashmap we also reset our memory context to remove any left over references.
#7236) DESCRIPTION: Fix leaking of memory and memory contexts in Foreign Constraint Graphs Previously, every time we (re)created the Foreign Constraint Relationship Graph, we created a new Memory Context while loosing a reference to the previous context. This old context could still have left over memory in there causing a memory leak. With this patch we statically have one memory context that we lazily initialize the first time we create our foreign constraint relationship graph. On every subsequent creation, beside destroying our previous hashmap we also reset our memory context to remove any left over references.
#7236) DESCRIPTION: Fix leaking of memory and memory contexts in Foreign Constraint Graphs Previously, every time we (re)created the Foreign Constraint Relationship Graph, we created a new Memory Context while loosing a reference to the previous context. This old context could still have left over memory in there causing a memory leak. With this patch we statically have one memory context that we lazily initialize the first time we create our foreign constraint relationship graph. On every subsequent creation, beside destroying our previous hashmap we also reset our memory context to remove any left over references.
#7236) DESCRIPTION: Fix leaking of memory and memory contexts in Foreign Constraint Graphs Previously, every time we (re)created the Foreign Constraint Relationship Graph, we created a new Memory Context while loosing a reference to the previous context. This old context could still have left over memory in there causing a memory leak. With this patch we statically have one memory context that we lazily initialize the first time we create our foreign constraint relationship graph. On every subsequent creation, beside destroying our previous hashmap we also reset our memory context to remove any left over references.
#7236) DESCRIPTION: Fix leaking of memory and memory contexts in Foreign Constraint Graphs Previously, every time we (re)created the Foreign Constraint Relationship Graph, we created a new Memory Context while loosing a reference to the previous context. This old context could still have left over memory in there causing a memory leak. With this patch we statically have one memory context that we lazily initialize the first time we create our foreign constraint relationship graph. On every subsequent creation, beside destroying our previous hashmap we also reset our memory context to remove any left over references.
#7236) DESCRIPTION: Fix leaking of memory and memory contexts in Foreign Constraint Graphs Previously, every time we (re)created the Foreign Constraint Relationship Graph, we created a new Memory Context while loosing a reference to the previous context. This old context could still have left over memory in there causing a memory leak. With this patch we statically have one memory context that we lazily initialize the first time we create our foreign constraint relationship graph. On every subsequent creation, beside destroying our previous hashmap we also reset our memory context to remove any left over references.
Co-authored-by: Onur Tirtir <onurcantirtir@gmail.com> (cherry picked from commit 92228b2)
DESCRIPTION: Fix leaking of memory and memory contexts in Foreign Constraint Graphs
Previously, every time we (re)created the Foreign Constraint Relationship Graph, we created a new Memory Context while loosing a reference to the previous context. This old context could still have left over memory in there causing a memory leak.
With this patch we statically have one memory context that we lazily initialize the first time we create our foreign constraint relationship graph. On every subsequent creation, beside destroying our previous hashmap we also reset our memory context to remove any left over references.