Harden against Postgres interactions with C++ destructors

[Y--]

Fixes https://github.com/duckdb/pg_duckdb/issues/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)

[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]

Closing this in favor of all the smaller PRs that we've been doing to address this problem step-by-step

[Y--]

Closing this in favor of all the smaller PRs that we've been doing to address this problem step-by-step

Yeah I was going to re-use this as the final PR but it doesn't really matter :-)