Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor conversionstate cleanup in Hypercore TAM #7475

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+80 −21
Open

Refactor conversionstate cleanup in Hypercore TAM #7475

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 80 additions & 21 deletions tsl/src/hypercore/hypercore_handler.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1471,6 +1471,8 @@
Oid relid;
RelationSize before_size;
Tuplesortstate *tuplesortstate;
MemoryContext mcxt;
MemoryContextCallback cb;
} ConversionState;

static ConversionState *conversionstate = NULL;
Expand Down Expand Up @@ -3353,6 +3355,77 @@
return false;
}

/*
* Cleanup callback for Hypercore conversion state.
*
* The cleanup happens when the conversion state's memory context is
* destroyed. It ensures cleanup of tuplesort state, unless it was already
* performed.
*/
static void
conversionstate_cleanup(void *arg)
{
ConversionState *state = arg;

if (state->tuplesortstate)
{
tuplesort_end(state->tuplesortstate);
state->tuplesortstate = NULL;

Check warning on line 3373 in tsl/src/hypercore/hypercore_handler.c

View check run for this annotation

Codecov / codecov/patch

tsl/src/hypercore/hypercore_handler.c#L3372-L3373 

Added lines #L3372 - L3373 were not covered by tests
}
Comment on lines +3370 to +3374
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@erimatnor erimatnor Nov 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


if (conversionstate)
{
Assert(state == conversionstate);
conversionstate = NULL;
}
}

static ConversionState *
conversionstate_create(const HypercoreInfo *hcinfo, const Relation rel)
{
CompressionSettings *settings = ts_compression_settings_get(hcinfo->compressed_relid);
Tuplesortstate *tuplesortstate;
MemoryContext mcxt;
MemoryContext oldmcxt;
ConversionState *state;

oldmcxt = MemoryContextSwitchTo(PortalContext);
/*
* We want to ensure the tuplesort state is cleaned up by calling
* tuplesort_end() in case of failures. This is necessary to release disk
* resources.
*
* A memory context callback is used for this purpose. The callback is
* attached to the Hypercore conversion state. The tuplesort state will
* allocate its own child context, but they cannot be children of the
* conversion memory context because children are freed before the
* 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.
Comment on lines +3402 to +3405
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried it, and it is possible. But I ran into some strange issues. I can check again.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@mkindahl mkindahl Nov 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, IC. Thanks for checking!

*/
tuplesortstate = compression_create_tuplesort_state(settings, rel);
mcxt = AllocSetContextCreate(PortalContext, "Hypercore conversion", ALLOCSET_DEFAULT_SIZES);

state = MemoryContextAlloc(mcxt, sizeof(ConversionState));
state->mcxt = mcxt;
state->before_size = ts_relation_size_impl(RelationGetRelid(rel));
state->tuplesortstate = tuplesortstate;
Assert(state->tuplesortstate);
state->relid = RelationGetRelid(rel);
state->cb.arg = state;
/* MemoryContext trees are cleaned up bootom up, so children are freed
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/* MemoryContext trees are cleaned up bootom up, so children are freed
/* MemoryContext trees are cleaned up bottom up, so children are freed

* before parents. Prevent this from happening so that we can call
* tuplesort_end() to release disk resources before the tuplesort child is
* cleaned up. Instead, clean up the children in our callback. */
state->cb.func = conversionstate_cleanup;
conversionstate = state;
MemoryContextRegisterResetCallback(state->mcxt, &state->cb);
MemoryContextSwitchTo(oldmcxt);

return state;
}

/*
* Convert a table to Hypercore.
*
Expand All @@ -3363,7 +3436,7 @@
{
Relation relation = table_open(relid, AccessShareLock);
bool compress_chunk_created;
HypercoreInfo *hsinfo = lazy_build_hypercore_info_cache(relation,
HypercoreInfo *hcinfo = lazy_build_hypercore_info_cache(relation,
false /* create constraints */,
&compress_chunk_created);

Expand All @@ -3372,21 +3445,13 @@
/* A compressed relation already exists, so converting from legacy
* compression. It is only necessary to create the proxy vacuum
* index. */
create_proxy_vacuum_index(relation, hsinfo->compressed_relid);
create_proxy_vacuum_index(relation, hcinfo->compressed_relid);

Check warning on line 3448 in tsl/src/hypercore/hypercore_handler.c

View check run for this annotation

Codecov / codecov/patch

tsl/src/hypercore/hypercore_handler.c#L3448 

Added line #L3448 was not covered by tests
table_close(relation, AccessShareLock);
return;
erimatnor marked this conversation as resolved.
Show resolved Hide resolved
}

MemoryContext oldcxt = MemoryContextSwitchTo(CacheMemoryContext);
ConversionState *state = palloc0(sizeof(ConversionState));
CompressionSettings *settings = ts_compression_settings_get(hsinfo->compressed_relid);
state->before_size = ts_relation_size_impl(relid);
state->tuplesortstate = compression_create_tuplesort_state(settings, relation);
Assert(state->tuplesortstate);
state->relid = relid;
conversionstate = state;
MemoryContextSwitchTo(oldcxt);
table_close(relation, AccessShareLock);
conversionstate = conversionstate_create(hcinfo, relation);
table_close(relation, NoLock);
}

/*
Expand Down Expand Up @@ -3465,14 +3530,6 @@
list_free(cleanup_relids);
cleanup_relids = NIL;
}

if (conversionstate)
{
if (conversionstate->tuplesortstate)
tuplesort_end(conversionstate->tuplesortstate);
pfree(conversionstate);
conversionstate = NULL;
}
}

static void
Expand Down Expand Up @@ -3559,7 +3616,9 @@
row_compressor.num_compressed_rows,
row_compressor.num_compressed_rows);

conversionstate = NULL;
MemoryContextDelete(conversionstate->mcxt);
/* Deleting the memorycontext should reset the global conversionstate pointer to NULL */
Assert(conversionstate == NULL);
}

/*
Expand Down
Loading