Harden against Postgres interactions with C++ destructors

[Y--]

Fixes #93

• make sure that no function uses elog(ERROR... which would bypass C++ dtors, and instead throw regular exceptions
• wraps every PG hooks in a try/catch block that converts the C++ exception to an PG equivalent (elog(ERROR, ... form)

[hlinnaka]

At quick glance, there are calls to get_namespace_oid() and get_relname_relid() in DuckDBManager::CheckSecretsSeq() that also seem unsafe.

Is there a general plan for how to sanity check that each function is handling errors correctly. If I understand correctly, some code runs in "Postgres regime", where you use elog(ERROR), memory context etc, while other code runs in "duckdb regime" where you use c++ exceptions etc. And whenever you cross the boundary, you must use PostgresFunctionGuard or DuckDBFunctionGuard.

Can we have some kind of compiler support or conventions or something to help with getting that right?

[hlinnaka]

I think DuckDBManager::DropSecrets needs a similar change

[Y--]

At quick glance, there are calls to get_namespace_oid() and get_relname_relid() in DuckDBManager::CheckSecretsSeq() that also seem unsafe.

Is there a general plan for how to sanity check that each function is handling errors correctly. If I understand correctly, some code runs in "Postgres regime", where you use elog(ERROR), memory context etc, while other code runs in "duckdb regime" where you use c++ exceptions etc. And whenever you cross the boundary, you must use PostgresFunctionGuard or DuckDBFunctionGuard.

Can we have some kind of compiler support or conventions or something to help with getting that right?

Thanks for the review @hlinnaka ! There are indeed a lot more places that requires attention. (I didn't realize the PR wasn't in draft - updated now)

My goal is to get to a place where we:

• use C++ as much as possible in this codebase
• wrap non-trivial PG function calls with PostgresFunctionGuard to catch PG errors and throw C++ exceptions
• wrap every top-level function (hooks, etc.) to catch C++ exceptions and convert back to PG errors

What do you think about this approach?

[hlinnaka]

My goal is to get to a place where we:

• use C++ as much as possible in this codebase

• wrap non-trivial PG function calls with PostgresFunctionGuard to catch PG errors and throw C++ exceptions

• wrap every top-level function (hooks, etc.) to catch C++ exceptions and convert back to PG errors

What do you think about this approach?

Yep, that works. Greenplum's orca optimizer does something similar, see e.g. https://github.com/cloudberrydb/cloudberrydb/blob/main/src/backend/gpopt/gpdbwrappers.cpp. I think PostGIS too.

[JelteF]

Moved this to the 0.2.0 milestone, to allow us to focus on stability testing for 0.1.0. We might still merge part of this PR for 0.1.0 where necessary to resolve stability issues we find during testing, but for now merging this wholesale so close to the release seems more risky than postponing it. We'll try to merge this (and related fixes) early in the 0.2.0 release cycle so it can bake longer.

[JelteF]

Could be converted to SPI_exec_or_throw afaict.

[Y--]

Hmm don't we want to reset the user id etc (L558-559) before throwing? I thought about adding a version with a callback to execute before throwing but that felt overkill.

[JelteF]

I think we discussed this in person, but I'll write it down here as well:

I'm not entirely convinced yet that changing all the elogs to exceptions in this file and in pgduckdb_ruleutils.cpp is all that helpful. It doesn't hurt either functionally, because you've wrapped this fuction in an InvokeCPPFunc call, and there's no PG_TRY around them. So there is no problem with throwing exceptions now.

But I think these specific functions probably won't benefit from being rewritten to C++ completely, and would be easier to maintain in Postgres C style. Mainly because they call a lot of Postgres functions and very few DuckDB functions. Wrapping all of those Postgres calls in PostgresFunctionGuard calls doesn't necessarily sound like it makes the code easier to read and maintain. I'm happily proven wrong though.

If you're fairly sure this is the right direction for these functions I'm fine with merging this now. But I think it should be a separate commit from most of the other changes (specifically the new and improved macros). To make reverting it easier if it turns out we want to make these functions fully Postgres C instead of fully C++.