Refactor conversionstate cleanup in Hypercore TAM #7475
Conversation
erimatnor
force-pushed
the
refactor-hypercore-conversionstate
branch
from
ae5cd83
to
47ff474
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7475 +/- ##
==========================================
+ Coverage 80.06% 82.17% +2.10%
==========================================
Files 190 230 +40
Lines 37181 43143 +5962
Branches 9450 10848 +1398
==========================================
+ Hits 29770 35453 +5683
- Misses 2997 3373 +376
+ Partials 4414 4317 -97 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
When converting a table to using Hypercore TAM, the table is rewritten using a tuple sort state. This state was allocated on the CacheMemoryContext and cleaned up in a transaction callback in case of abort. Instead, allocate the conversion state in its own memory context, which is a child of PortalContext. That makes it possible to do clean up in a memory context callback that guarantees cleanup when the conversion command is complete or when a failure occurs.
erimatnor
force-pushed
the
refactor-hypercore-conversionstate
branch
from
47ff474
to
bcf3837
Compare
| if (state->tuplesortstate) | ||
| { | ||
| tuplesort_end(state->tuplesortstate); | ||
| state->tuplesortstate = NULL; | ||
| } |
There was a problem hiding this comment.
Is it possible to test this code some way? I am not entirely sure why this is not covered by normal execution though, so might not be possible.
There was a problem hiding this comment.
I did it manually by injecting a elog(ERROR) in places where the tuplesort state is not ended. It works for those cases. Not sure how to inject errors in random places for a specific test. Any suggestions?
| * parent. Instead, make both the tuplesort and conversion state children | ||
| * of PortalContext. Since they are destroyed in reverse order, the memory | ||
| * context callback for the conversion state can call tuplesort_end() | ||
| * before the tuplesort is freed. |
There was a problem hiding this comment.
So, if I read this right the tuplesort and conversion state are "siblings" and in that case the order of the destruction is not imposed by the structure of the parent-child relationship, which means that the order can change if modifications are done here (for rational reasons or inadvertently).
Is it possible to make the conversion state a child of the tuplesort state? In that case the ordering would always be correct since the child context (that is, the conversion state) needs to be destroyed before the parent state (that is, the tuplesort state), but I am not sure if that leads to other problems.
There was a problem hiding this comment.
I tried it, and it is possible. But I ran into some strange issues. I can check again.
There was a problem hiding this comment.
So, the reason why making it a child is that it creates a circular memory release problem.
Basically, when tuplesort_end() is called due to normal processing, it will destroy its memory context, including child contexts first. But if the child then also calls tuplesort_end() to clean up the parent it doesn't work.
There was a problem hiding this comment.
Ah, IC. Thanks for checking!
| Assert(state->tuplesortstate); | ||
| state->relid = RelationGetRelid(rel); | ||
| state->cb.arg = state; | ||
| /* MemoryContext trees are cleaned up bootom up, so children are freed |
There was a problem hiding this comment.
| /* MemoryContext trees are cleaned up bootom up, so children are freed | |
| /* MemoryContext trees are cleaned up bottom up, so children are freed |
When converting a table to using Hypercore TAM, the table is rewritten using a tuple sort state. This state was allocated on the CacheMemoryContext and cleaned up in a transaction callback in case of abort.
Instead, allocate the conversion state in its own memory context, which is a child of PortalContext. That makes it possible to do clean up in a memory context callback that guarantees cleanup when the conversion command is complete or when a failure occurs.
Disable-check: force-changelog-file