Review feedback changes

CONTRIBUTING.md

@@ -128,10 +128,11 @@ coding styles.
 ## Error Handling
 
 * Use exceptions **only** when an error is encountered that terminates a query (e.g. parser error, table not found). Exceptions should only be used for **exceptional** situations. For regular errors that do not break the execution flow (e.g. errors you **expect** might occur) use a return value instead.
-* *Exception* should be used in code that is executed inside duckdb engine.
-* Use PostgreSQL *elog* API to report messages back to user. Using *ERROR* is strictly forbiden to use as it can brake execution and lead to
-unexpected memory problems - *ERROR* is only used in duckdb node to early bail out execution in case of query preparation error or execution interruption.
-* Calling PostgreSQL native functions needs to be used with *PostgresFunctionGuard*. *PostgresFunctionGuard* will handle correctly *ERROR* log messages that could be emmited from these functions.
+* There are two distinct parts of the code where error handling is done very differently: The code that executes before we enter DuckDB execution engine (e.g. initial part of the planner hook) and the part that gets executed inside the duckdb execution engine. Below are rules for how to handle errors in both parts of the code. Not following these guidelines can cause crashes, memory leaks and other unexpected problems.
+* Before we enter the DuckDB exection engine no exceptions should ever be thrown here. In cases where you would want to throw an exception here, use `elog(ERROR, ...)`. Any C++ code that might throw an exception is also problematic. Since C++ throws exceptions on allocation failures, this covers lots of C++ APIs. So try to use Postgres datastructures instead of C++ ones whenever possible (e.g. use `List` instead of `Vec`)
+* Inside the duckdb execution engine the opposite is true. `elog(ERROR, ...)` should never be used there, use exceptions instead.
+* Use PostgreSQL *elog* API can be used to report non-fatal messages back to user. Using *ERROR* is strictly forbiden to use in code that is executed inside the duckdb engine.
+* Calling PostgreSQL native functions from within DuckDB execution needs **extreme care**. Pretty much non of these functions are thread-safe, and they might throw errors using `elog(ERROR, ...)`. If you've solved the thread-safety issue by taking a lock (or by carefully asserting that the actual code is thread safe), then you can use *PostgresFunctionGuard* to solve the `elog(ERROR, ...) problem. *PostgresFunctionGuard* will correctly handle *ERROR* log messages that could be emmited from these functions.
 * Try to add test cases that trigger exceptions. If an exception cannot be easily triggered using a test case then it should probably be an assertion. This is not always true (e.g. out of memory errors are exceptions, but are very hard to trigger).
 * Use `D_ASSERT` to assert. Use **assert** only when failing the assert means a programmer error. Assert should never be triggered by user input. Avoid code like `D_ASSERT(a > b + 3);` without comments or context.
 * Assert liberally, but make it clear with comments next to the assert what went wrong when the assert is triggered.

include/pgduckdb/pgduckdb_utils.hpp

@@ -23,18 +23,29 @@ TokenizeString(char *str, const char delimiter) {
 	return v;
 };
 
+/*
+ * DuckdbGlobalLock should be held before calling.
+ */
 template <typename T, typename FuncType, typename... FuncArgs>
 std::optional<T>
 PostgresFunctionGuard(FuncType postgres_function, FuncArgs... args) {
 	T return_value;
 	bool error = false;
+	MemoryContext ctx = CurrentMemoryContext;
 	// clang-format off
 	PG_TRY();
 	{
 		return_value = postgres_function(args...);
 	}
 	PG_CATCH();
 	{
+		ErrorData  *edata;
+		MemoryContextSwitchTo(ctx);
+		edata = CopyErrorData();
+		FlushErrorState();
+		ereport(DEBUG1,
+				(errcode(ERRCODE_INTERNAL_ERROR),
+				 errmsg("(DuckDB/PostgresFunctionGuard) %s", edata->message)));
 		error = true;
 	}
 	PG_END_TRY();
@@ -49,13 +60,21 @@ template <typename FuncType, typename... FuncArgs>
 std::optional<bool>
 PostgresFunctionGuard(FuncType postgres_function, FuncArgs... args) {
 	bool error = false;
+	MemoryContext ctx = CurrentMemoryContext;
 	// clang-format off
 	PG_TRY();
 	{
 		postgres_function(args...);
 	}
 	PG_CATCH();
 	{
+		ErrorData  *edata;
+		MemoryContextSwitchTo(ctx);
+		edata = CopyErrorData();
+		FlushErrorState();
+		ereport(DEBUG1,
+				(errcode(ERRCODE_INTERNAL_ERROR),
+				 errmsg("(DuckDB/PostgresFunctionGuard) %s", edata->message)));
 		error = true;
 	}
 	PG_END_TRY();

src/pgduckdb_detoast.cpp

@@ -89,7 +89,6 @@ ToastDecompressDatum(struct varlena *attr) {
 
 static struct varlena *
 ToastFetchDatum(struct varlena *attr) {
-	Relation toast_rel;
 	struct varlena *result;
 	struct varatt_external toast_pointer;
 	int32 attrsize;
@@ -115,25 +114,22 @@ ToastFetchDatum(struct varlena *attr) {
 		return result;
 	}
 
-	DuckdbProcessLock::GetLock().lock();
-	toast_rel = try_table_open(toast_pointer.va_toastrelid, AccessShareLock);
+	std::lock_guard<std::mutex> lock(DuckdbProcessLock::GetLock());
 
-	if (toast_rel != NULL) {
-		DuckdbProcessLock::GetLock().unlock();
+	auto toast_rel = PostgresFunctionGuard<Relation>(try_table_open, toast_pointer.va_toastrelid, AccessShareLock);
+
+	if (!toast_rel.has_value() || toast_rel.value() == NULL) {
 		throw duckdb::InternalException("(PGDuckDB/ToastFetchDatum) Error opening toast relation");
 	}
 
-	bool error_fetch_toast = false;
-	if (PostgresFunctionGuard(table_relation_fetch_toast_slice, toast_rel, toast_pointer.va_valueid, attrsize, 0,
-	                          attrsize, result).value()) {
-		error_fetch_toast = true;
+	if (PostgresFunctionGuard(table_relation_fetch_toast_slice, toast_rel.value(), toast_pointer.va_valueid, attrsize,
+	                          0, attrsize, result)
+	        .value()) {
+		throw duckdb::InternalException("(PGDuckDB/ToastFetchDatum) Error reading external toast table");
 	}
 
-	table_close(toast_rel, AccessShareLock);
-	DuckdbProcessLock::GetLock().unlock();
-
-	if (error_fetch_toast) {
-		throw duckdb::InternalException("(PGDuckDB/ToastFetchDatum) Error reading external toast table");
+	if (PostgresFunctionGuard(table_close, toast_rel.value(), AccessShareLock).value()) {
+		throw duckdb::InternalException("(PGDuckDB/ToastFetchDatum) Error closing external toast table");
 	}
 
 	return result;

src/scan/heap_reader.cpp

@@ -87,25 +87,22 @@ HeapReader::ReadPageTuples(duckdb::DataChunk &output) {
 	while (block != InvalidBlockNumber) {
 		if (m_read_next_page) {
 			CHECK_FOR_INTERRUPTS();
-			DuckdbProcessLock::GetLock().lock();
+			std::lock_guard<std::mutex> lock(DuckdbProcessLock::GetLock());
 			block = m_block_number;
 
 			auto opt_buffer = PostgresFunctionGuard<Buffer>(ReadBufferExtended, m_relation, MAIN_FORKNUM, block,
 			                                                RBM_NORMAL, GetAccessStrategy(BAS_BULKREAD));
 
 			if (!opt_buffer.has_value()) {
-				DuckdbProcessLock::GetLock().unlock();
 				throw duckdb::InternalException("(PGDuckdDB/ReadPageTuples) ReadBufferExtended failed");
 			}
 
 			m_buffer = opt_buffer.value();
 
 			if (PostgresFunctionGuard(LockBuffer, m_buffer, BUFFER_LOCK_SHARE).value()) {
-				DuckdbProcessLock::GetLock().unlock();
 				throw duckdb::InternalException("(PGDuckdDB/ReadPageTuples) LockBuffer failed");
 			}
 
-			DuckdbProcessLock::GetLock().unlock();
 			page = PreparePageRead();
 			m_read_next_page = false;
 		}