From 77c2698f72e8b80d82e9cf6d5f82f70aaa3ce14a Mon Sep 17 00:00:00 2001 From: mkaruza Date: Mon, 16 Sep 2024 09:36:57 +0200 Subject: [PATCH] Extend CONTRIBUTING.md with error handling information * Add few informations how we should handle errors inside code. * Use same function name `PostgresFunctionGuard` for both void and non-void PG function calls. --- CONTRIBUTING.md | 4 ++++ include/pgduckdb/pgduckdb_utils.hpp | 4 ++-- src/pgduckdb_detoast.cpp | 4 ++-- src/scan/heap_reader.cpp | 2 +- 4 files changed, 9 insertions(+), 5 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 47f16ae0..ee748c8a 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -128,6 +128,10 @@ 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. * 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. diff --git a/include/pgduckdb/pgduckdb_utils.hpp b/include/pgduckdb/pgduckdb_utils.hpp index bf157b89..c88bc74b 100644 --- a/include/pgduckdb/pgduckdb_utils.hpp +++ b/include/pgduckdb/pgduckdb_utils.hpp @@ -46,8 +46,8 @@ PostgresFunctionGuard(FuncType postgres_function, FuncArgs... args) { } template -bool -PostgresVoidFunctionGuard(FuncType postgres_function, FuncArgs... args) { +std::optional +PostgresFunctionGuard(FuncType postgres_function, FuncArgs... args) { bool error = false; // clang-format off PG_TRY(); diff --git a/src/pgduckdb_detoast.cpp b/src/pgduckdb_detoast.cpp index 8b635ded..6bc37f01 100644 --- a/src/pgduckdb_detoast.cpp +++ b/src/pgduckdb_detoast.cpp @@ -124,8 +124,8 @@ ToastFetchDatum(struct varlena *attr) { } bool error_fetch_toast = false; - if (PostgresVoidFunctionGuard(table_relation_fetch_toast_slice, toast_rel, toast_pointer.va_valueid, attrsize, 0, - attrsize, result)) { + if (PostgresFunctionGuard(table_relation_fetch_toast_slice, toast_rel, toast_pointer.va_valueid, attrsize, 0, + attrsize, result).value()) { error_fetch_toast = true; } diff --git a/src/scan/heap_reader.cpp b/src/scan/heap_reader.cpp index 94d4736f..079dc99e 100644 --- a/src/scan/heap_reader.cpp +++ b/src/scan/heap_reader.cpp @@ -100,7 +100,7 @@ HeapReader::ReadPageTuples(duckdb::DataChunk &output) { m_buffer = opt_buffer.value(); - if (PostgresVoidFunctionGuard(LockBuffer, m_buffer, BUFFER_LOCK_SHARE)) { + if (PostgresFunctionGuard(LockBuffer, m_buffer, BUFFER_LOCK_SHARE).value()) { DuckdbProcessLock::GetLock().unlock(); throw duckdb::InternalException("(PGDuckdDB/ReadPageTuples) LockBuffer failed"); }