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

Fix memory leaks in custom executor #251

Merged
merged 7 commits into from
Oct 7, 2024
Merged

Fix memory leaks in custom executor #251

merged 7 commits into from
Oct 7, 2024

Conversation

Y--
Copy link
Collaborator

@Y-- Y-- commented Oct 4, 2024 •
edited
Loading

When one calls elog(ERROR,... it longjmp out of the current stack leaving no chance for the C++ to be executed.

This PR is a first iteration toward what we want to rationalize (cf. #93 ):

  • keep a function with a pure C++ "flavor" (object creation, exceptions, etc.)
  • call it in a new wrapper that copies out the error message and uses PG mechanism to throw the error

This fixes #120, even though there still seem to be a small leak with DuckDB's PrepareQuery leftover (to be followed-up separately)

@Y-- Y-- requested a review from JelteF October 4, 2024 14:50
Tishj
Tishj reviewed Oct 4, 2024
View reviewed changes
src/pgduckdb_node.cpp Outdated
}
}

auto pending = prepared.PendingQuery(duckdb_params, true);
if (pending->HasError()) {
elog(ERROR, "DuckDB execute returned an error: %s", pending->GetError().c_str());
throw std::runtime_error(pending->GetError());
Copy link
Collaborator

@Tishj Tishj Oct 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Can use pending->ThrowError() instead

That would also preserve the duckdb exception type and the information the exception holds, rather than stringifying it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's what I did initially but sadly it throws an ugly JSON then... Maybe I missed something, I'll try again.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried again and I got something like this:

ERROR:  (PGDuckDB/ExecuteQuery) {"exception_type":"Conversion","exception_message":"Could not convert string 'not an integer' to INT32\nLINE 1: SELECT (t)::integer AS t198 FROM public.foo\n                  ^","position":"10"}

Is there something specific to do so it doesn't output JSON?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Tishj I've pushed that commit to support the pending->ThrowError().
Maybe I missed something from DuckDB API, but I'm a bit surprised that:

  • I have to specially handle duckdb::Exception
  • ThrowError (in some cases at least) throws a std::exception, not a duckdb::Exception one

Happy to improve if there's a better way?

Tishj
Tishj approved these changes Oct 4, 2024
View reviewed changes
Copy link
Collaborator

@Tishj Tishj left a comment

Choose a reason for hiding this comment

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

Thanks, if I'm reading this correctly, we add DuckDBFunctionGuard to catch a c++ exception so we can perform cleanup on the postgres side before propagating this error upward?

Tishj
Tishj reviewed Oct 4, 2024
View reviewed changes
src/pgduckdb_node.cpp Outdated
if (execution_result == duckdb::PendingExecutionResult::EXECUTION_ERROR) {
CleanupDuckdbScanState(state);
elog(ERROR, "(PGDuckDB/ExecuteQuery) %s", pending->GetError().c_str());
throw std::runtime_error(pending->GetError());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comment as above

@Y--
Copy link
Collaborator Author

Y-- commented Oct 4, 2024

Thanks, if I'm reading this correctly, we add DuckDBFunctionGuard to catch a c++ exception so we can perform cleanup on the postgres side before propagating this error upward?

Yep, that's correct!

@JelteF JelteF added the bug Something isn't working label Oct 7, 2024
@Y-- Y-- merged commit 59ada4b into main Oct 7, 2024
4 checks passed
@Y-- Y-- deleted the yl/fix-memory-leak branch October 7, 2024 12:27
Y-- added a commit that referenced this pull request Oct 7, 2024
@Y--
Revert "Fix memory leaks in custom executor (#251)"
c43e429 
This reverts commit 59ada4b.
JelteF pushed a commit that referenced this pull request Oct 7, 2024
@Y--
Revert "Fix memory leaks in custom executor (#251)" (#255)
a690e27 
This reverts commit 59ada4b.

Seems that the previous PR is causing issues like:
```
2024-10-07 15:55:16.797 CEST client backend[159884] pg_regress/execution_error STATEMENT:  select a::INTEGER from int_as_varchar;
TRAP: failed Assert("rel->rd_refcnt > 0"), File: "relcache.c", Line: 2176, PID: 159884
postgres: jelte regression [local] idle(ExceptionalCondition+0x6e)[0x5ff6eb3300a4]
postgres: jelte regression [local] idle(RelationDecrementReferenceCount+0x38)[0x5ff6eb323998]
postgres: jelte regression [local] idle(RelationClose+0x15)[0x5ff6eb3239c5]
/home/jelte/.pgenv/pgsql-17beta9/lib/pg_duckdb.so(+0x2b836)[0x7c5b33ae6836]
/home/jelte/.pgenv/pgsql-17beta9/lib/pg_duckdb.so(+0x2bc7e)[0x7c5b33ae6c7e]
/home/jelte/.pgenv/pgsql-17beta9/lib/pg_duckdb.so(+0x2bdbd)[0x7c5b33ae6dbd]
/home/jelte/.pgenv/pgsql-17beta9/lib/pg_duckdb.so(+0x2e015)[0x7c5b33ae9015]
/home/jelte/.pgenv/pgsql-17beta9/lib/libduckdb.so(_ZN6duckdb16AttachedDatabaseD1Ev+0x2d)[0x7c5b2f707edd]
/home/jelte/.pgenv/pgsql-17beta9/lib/libduckdb.so(_ZN6duckdb16AttachedDatabaseD0Ev+0xd)[0x7c5b2f707f1d]
/home/jelte/.pgenv/pgsql-17beta9/lib/libduckdb.so(+0x1409628)[0x7c5b2f009628]
/home/jelte/.pgenv/pgsql-17beta9/lib/libduckdb.so(_ZN6duckdb10CatalogSetD1Ev+0x2e)[0x7c5b2f00c66e]
/home/jelte/.pgenv/pgsql-17beta9/lib/libduckdb.so(_ZN6duckdb15DatabaseManager14ResetDatabasesERNS_10unique_ptrINS_13TaskSchedulerESt14default_deleteIS2_ELb1EEE+0xfd)[0x7c5b2f70830d]
/home/jelte/.pgenv/pgsql-17beta9/lib/libduckdb.so(_ZN6duckdb16DatabaseInstanceD1Ev+0x25)[0x7c5b2f70b5c5]
/home/jelte/.pgenv/pgsql-17beta9/lib/libduckdb.so(_ZNSt16_Sp_counted_baseILN9__gnu_cxx12_Lock_policyE2EE24_M_release_last_use_coldEv+0xe)[0x7c5b2e5d9a6e]
/home/jelte/.pgenv/pgsql-17beta9/lib/pg_duckdb.so(+0x1a6f5)[0x7c5b33ad56f5]
/lib/x86_64-linux-gnu/libc.so.6(+0x47a66)[0x7c5b32e47a66]
/lib/x86_64-linux-gnu/libc.so.6(+0x47bae)[0x7c5b32e47bae]
postgres: jelte regression [local] idle(proc_exit+0x3d)[0x5ff6eb1b013a]
postgres: jelte regression [local] idle(PostgresMain+0x37e)[0x5ff6eb1e0ffe]
postgres: jelte regression [local] idle(BackendMain+0x51)[0x5ff6eb1da61b]
postgres: jelte regression [local] idle(postmaster_child_launch+0xc7)[0x5ff6eb131860]
postgres: jelte regression [local] idle(+0x444029)[0x5ff6eb136029]
postgres: jelte regression [local] idle(+0x4442ac)[0x5ff6eb1362ac]
postgres: jelte regression [local] idle(BackgroundWorkerInitializeConnection+0x0)[0x5ff6eb137920]
postgres: jelte regression [local] idle(main+0x20e)[0x5ff6eb053282]
/lib/x86_64-linux-gnu/libc.so.6(+0x2a1ca)[0x7c5b32e2a1ca]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0x8b)[0x7c5b32e2a28b]
postgres: jelte regression [local] idle(_start+0x25)[0x5ff6eadcfef5]
```
Reverting to investigate
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Tishj Tishj Tishj approved these changes

@JelteF JelteF JelteF approved these changes

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Resource leak on error, eventual crash on out of memory
3 participants
@Y-- @JelteF @Tishj