Skip to content

Commit

Permalink
Extend CONTRIBUTING.md with error handling information
Browse files Browse the repository at this point in the history
* Add few informations how we should handle errors inside code.
* Use same function name `PostgresFunctionGuard` for both void and
  non-void PG function calls.
  • Loading branch information
mkaruza committed Sep 16, 2024
1 parent 9dbbcf6 commit 77c2698
Show file tree
Hide file tree
Showing 4 changed files with 9 additions and 5 deletions.
4 changes: 4 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
4 changes: 2 additions & 2 deletions include/pgduckdb/pgduckdb_utils.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,8 @@ PostgresFunctionGuard(FuncType postgres_function, FuncArgs... args) {
}

template <typename FuncType, typename... FuncArgs>
bool
PostgresVoidFunctionGuard(FuncType postgres_function, FuncArgs... args) {
std::optional<bool>
PostgresFunctionGuard(FuncType postgres_function, FuncArgs... args) {
bool error = false;
// clang-format off
PG_TRY();
Expand Down
4 changes: 2 additions & 2 deletions src/pgduckdb_detoast.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
2 changes: 1 addition & 1 deletion src/scan/heap_reader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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");
}
Expand Down

0 comments on commit 77c2698

Please sign in to comment.