From 2c0729cc2bea3b1a1fe5c957b7611ea1586cc041 Mon Sep 17 00:00:00 2001 From: Yves Date: Thu, 10 Oct 2024 14:34:18 +0200 Subject: [PATCH] Transform `elog(ERROR` to C++ exceptions --- include/pgduckdb/pgduckdb_duckdb.hpp | 4 +- include/pgduckdb/pgduckdb_ruleutils.h | 1 - include/pgduckdb/pgduckdb_utils.hpp | 22 +++++++- src/pgduckdb_ddl.cpp | 80 +++++++++++---------------- src/pgduckdb_duckdb.cpp | 8 +-- src/pgduckdb_hooks.cpp | 7 ++- src/pgduckdb_options.cpp | 3 +- src/pgduckdb_planner.cpp | 2 +- src/pgduckdb_ruleutils.cpp | 49 +++++++--------- 9 files changed, 82 insertions(+), 94 deletions(-) diff --git a/include/pgduckdb/pgduckdb_duckdb.hpp b/include/pgduckdb/pgduckdb_duckdb.hpp index 34bb9f18..d623de2e 100644 --- a/include/pgduckdb/pgduckdb_duckdb.hpp +++ b/include/pgduckdb/pgduckdb_duckdb.hpp @@ -26,8 +26,8 @@ class DuckDBManager { return *Get().database; } - static duckdb::unique_ptr - CreateConnection(); + static duckdb::unique_ptr CreateConnection(); + private: DuckDBManager(); diff --git a/include/pgduckdb/pgduckdb_ruleutils.h b/include/pgduckdb/pgduckdb_ruleutils.h index ca4b6e96..d6fdce1a 100644 --- a/include/pgduckdb/pgduckdb_ruleutils.h +++ b/include/pgduckdb/pgduckdb_ruleutils.h @@ -1,5 +1,4 @@ #include "postgres.h" char *pgduckdb_relation_name(Oid relid); -char *pgduckdb_function_name(Oid function_oid); char *pgduckdb_get_tabledef(Oid relation_id); diff --git a/include/pgduckdb/pgduckdb_utils.hpp b/include/pgduckdb/pgduckdb_utils.hpp index 84bab9b6..02b1eaff 100644 --- a/include/pgduckdb/pgduckdb_utils.hpp +++ b/include/pgduckdb/pgduckdb_utils.hpp @@ -78,7 +78,7 @@ PostgresFunctionGuard(FuncType postgres_function, FuncArgs... args) { template FuncRetT -DuckDBFunctionGuard(FuncType duckdb_function, const char* function_name, FuncArgs... args) { +DuckDBFunctionGuard(FuncType duckdb_function, const char *function_name, FuncArgs... args) { const char *error_message = nullptr; try { return duckdb_function(args...); @@ -111,4 +111,24 @@ DuckDBQueryOrThrow(duckdb::ClientContext &context, const std::string &query) { return res; } +inline duckdb::unique_ptr +DuckDBQueryOrThrow(duckdb::Connection &connection, const std::string &query) { + auto res = connection.context->Query(query, false); + if (res->HasError()) { + res->ThrowError(); + } + return res; +} + +inline duckdb::unique_ptr +DuckDBQueryOrThrow(const std::string &query) { + auto connection = pgduckdb::DuckDBManager::CreateConnection(); + auto &context = *connection->context; + auto res = context.Query(query, false); + if (res->HasError()) { + res->ThrowError(); + } + return res; +} + } // namespace pgduckdb diff --git a/src/pgduckdb_ddl.cpp b/src/pgduckdb_ddl.cpp index 1f48e684..c58fc8da 100644 --- a/src/pgduckdb_ddl.cpp +++ b/src/pgduckdb_ddl.cpp @@ -22,6 +22,7 @@ extern "C" { } #include "pgduckdb/pgduckdb_duckdb.hpp" +#include "pgduckdb/pgduckdb_utils.hpp" #include "pgduckdb/vendor/pg_list.hpp" #include @@ -30,13 +31,7 @@ extern "C" { */ void DuckdbTruncateTable(Oid relation_oid) { - auto connection = pgduckdb::DuckDBManager::CreateConnection(); - auto &context = *connection->context; - auto query = std::string("TRUNCATE ") + pgduckdb_relation_name(relation_oid); - auto result = context.Query(query, false); - if (result->HasError()) { - elog(ERROR, "(PGDuckDB/DuckdbTruncateTable) Could not truncate table: %s", result->GetError().c_str()); - } + pgduckdb::DuckDBQueryOrThrow(std::string("TRUNCATE ") + pgduckdb_relation_name(relation_oid)); } void @@ -49,7 +44,7 @@ DuckdbHandleDDL(Node *parsetree, const char *queryString) { return; } - elog(ERROR, "DuckDB does not support CREATE TABLE AS yet"); + throw duckdb::NotImplementedException("DuckDB does not support CREATE TABLE AS yet"); } } @@ -69,9 +64,9 @@ CheckOnCommitSupport(OnCommitAction on_commit) { case ONCOMMIT_DELETE_ROWS: break; case ONCOMMIT_DROP: - elog(ERROR, "DuckDB does not support ON COMMIT DROP"); + throw duckdb::NotImplementedException("DuckDB does not support ON COMMIT DROP"); default: - elog(ERROR, "Unsupported ON COMMIT clause: %d", on_commit); + throw duckdb::NotImplementedException("Unsupported ON COMMIT clause: ", on_commit); } } @@ -80,8 +75,10 @@ extern "C" { PG_FUNCTION_INFO_V1(duckdb_create_table_trigger); Datum duckdb_create_table_trigger(PG_FUNCTION_ARGS) { - if (!CALLED_AS_EVENT_TRIGGER(fcinfo)) /* internal error */ - elog(ERROR, "not fired by event trigger manager"); + if (!CALLED_AS_EVENT_TRIGGER(fcinfo)) { + /* internal error */ + throw std::runtime_error("not fired by event trigger manager"); + } EventTriggerData *trigdata = (EventTriggerData *)fcinfo->context; Node *parsetree = trigdata->parsetree; @@ -121,8 +118,9 @@ duckdb_create_table_trigger(PG_FUNCTION_ARGS) { SetUserIdAndSecContext(saved_userid, sec_context); AtEOXact_GUC(false, save_nestlevel); - if (ret != SPI_OK_INSERT_RETURNING) - elog(ERROR, "SPI_exec failed: error code %s", SPI_result_code_string(ret)); + if (ret != SPI_OK_INSERT_RETURNING) { + throw std::runtime_error(std::string("SPI_exec failed: error code ") + SPI_result_code_string(ret)); + } /* if we inserted a row it was a duckdb table */ auto is_duckdb_table = SPI_processed > 0; @@ -132,14 +130,16 @@ duckdb_create_table_trigger(PG_FUNCTION_ARGS) { } if (SPI_processed != 1) { - elog(ERROR, "Expected single table to be created, but found %" PRIu64, SPI_processed); + throw std::runtime_error("Expected single table to be created, but found " + std::to_string(SPI_processed)); } + HeapTuple tuple = SPI_tuptable->vals[0]; bool isnull; Datum relid_datum = SPI_getbinval(tuple, SPI_tuptable->tupdesc, 1, &isnull); if (isnull) { - elog(ERROR, "Expected relid to be returned, but found NULL"); + throw std::runtime_error("Expected relid to be returned, but found NULL"); } + Oid relid = DatumGetObjectId(relid_datum); SPI_finish(); @@ -157,18 +157,10 @@ duckdb_create_table_trigger(PG_FUNCTION_ARGS) { auto stmt = castNode(CreateTableAsStmt, parsetree); CheckOnCommitSupport(stmt->into->onCommit); } else { - elog(ERROR, "Unexpected parsetree type: %d", nodeTag(parsetree)); - } - - std::string query_string(pgduckdb_get_tabledef(relid)); - - auto connection = pgduckdb::DuckDBManager::CreateConnection(); - auto &context = *connection->context; - auto result = context.Query(query_string, false); - if (result->HasError()) { - elog(ERROR, "(PGDuckDB/duckdb_create_table_trigger) Could not create table: %s", result->GetError().c_str()); + throw std::runtime_error("Unexpected parsetree type: " + std::to_string(nodeTag(parsetree))); } + pgduckdb::DuckDBQueryOrThrow(pgduckdb_get_tabledef(relid)); PG_RETURN_NULL(); } @@ -176,7 +168,7 @@ PG_FUNCTION_INFO_V1(duckdb_drop_table_trigger); Datum duckdb_drop_table_trigger(PG_FUNCTION_ARGS) { if (!CALLED_AS_EVENT_TRIGGER(fcinfo)) /* internal error */ - elog(ERROR, "not fired by event trigger manager"); + throw std::runtime_error("not fired by event trigger manager"); SPI_connect(); @@ -214,8 +206,9 @@ duckdb_drop_table_trigger(PG_FUNCTION_ARGS) { SetUserIdAndSecContext(saved_userid, sec_context); AtEOXact_GUC(false, save_nestlevel); - if (ret != SPI_OK_DELETE_RETURNING) - elog(ERROR, "SPI_exec failed: error code %s", SPI_result_code_string(ret)); + if (ret != SPI_OK_DELETE_RETURNING) { + throw std::runtime_error("SPI_exec failed: error code '" + std::string(SPI_result_code_string(ret)) + "'"); + } if (SPI_processed == 0) { /* No duckdb tables were dropped */ @@ -231,40 +224,29 @@ duckdb_drop_table_trigger(PG_FUNCTION_ARGS) { PreventInTransactionBlock(true, "DuckDB queries"); auto connection = pgduckdb::DuckDBManager::CreateConnection(); - auto &context = *connection->context; - auto result = context.Query("BEGIN TRANSACTION", false); - if (result->HasError()) { - elog(ERROR, "(PGDuckDB/duckdb_drop_table_trigger) Could not start transaction"); - } + pgduckdb::DuckDBQueryOrThrow(*connection, "BEGIN TRANSACTION"); - for (auto proc = 0; proc < SPI_processed; proc++) { + for (auto proc = 0; proc < SPI_processed; ++proc) { HeapTuple tuple = SPI_tuptable->vals[proc]; char *object_identity = SPI_getvalue(tuple, SPI_tuptable->tupdesc, 1); // TODO: Handle std::string creation in a safe way for allocation failures - auto result = context.Query("DROP TABLE IF EXISTS " + std::string(object_identity), false); - if (result->HasError()) { - elog(ERROR, "(PGDuckDB/duckdb_drop_table_trigger) Could not drop table %s: %s", object_identity, - result->GetError().c_str()); - } + pgduckdb::DuckDBQueryOrThrow(*connection, "DROP TABLE IF EXISTS " + std::string(object_identity)); } SPI_finish(); - result = context.Query("COMMIT", false); - if (result->HasError()) { - elog(ERROR, "(PGDuckDB/duckdb_drop_table_trigger) Could not commit transaction"); - } - + pgduckdb::DuckDBQueryOrThrow(*connection, "COMMIT"); PG_RETURN_NULL(); } PG_FUNCTION_INFO_V1(duckdb_alter_table_trigger); Datum duckdb_alter_table_trigger(PG_FUNCTION_ARGS) { - if (!CALLED_AS_EVENT_TRIGGER(fcinfo)) /* internal error */ - elog(ERROR, "not fired by event trigger manager"); + if (!CALLED_AS_EVENT_TRIGGER(fcinfo)) { /* internal error */ + throw std::runtime_error("not fired by event trigger manager"); + } SPI_connect(); @@ -308,7 +290,7 @@ duckdb_alter_table_trigger(PG_FUNCTION_ARGS) { AtEOXact_GUC(false, save_nestlevel); if (ret != SPI_OK_SELECT) - elog(ERROR, "SPI_exec failed: error code %s", SPI_result_code_string(ret)); + throw std::runtime_error("SPI_exec failed: error code '" + std::string(SPI_result_code_string(ret)) + "'"); /* if we inserted a row it was a duckdb table */ auto is_duckdb_table = SPI_processed > 0; @@ -317,7 +299,7 @@ duckdb_alter_table_trigger(PG_FUNCTION_ARGS) { PG_RETURN_NULL(); } - elog(ERROR, "DuckDB does not support ALTER TABLE yet"); + throw duckdb::NotImplementedException("DuckDB does not support ALTER TABLE yet"); PG_RETURN_NULL(); } diff --git a/src/pgduckdb_duckdb.cpp b/src/pgduckdb_duckdb.cpp index 6b17af92..8e27267f 100644 --- a/src/pgduckdb_duckdb.cpp +++ b/src/pgduckdb_duckdb.cpp @@ -167,13 +167,9 @@ DuckDBManager::LoadSecrets(duckdb::ClientContext &context) { void DuckDBManager::DropSecrets(duckdb::ClientContext &context) { - for (auto secret_id = 0; secret_id < secret_table_num_rows; secret_id++) { auto drop_secret_cmd = duckdb::StringUtil::Format("DROP SECRET pgduckb_secret_%d;", secret_id); - auto res = context.Query(drop_secret_cmd, false); - if (res->HasError()) { - elog(ERROR, "(PGDuckDB/DropSecrets) secret `%d` could not be dropped.", secret_id); - } + pgduckdb::DuckDBQueryOrThrow(drop_secret_cmd); } secret_table_num_rows = 0; } @@ -191,7 +187,7 @@ DuckDBManager::LoadExtensions(duckdb::ClientContext &context) { duckdb::unique_ptr DuckDBManager::CreateConnection() { - auto& instance = Get(); + auto &instance = Get(); auto connection = duckdb::make_uniq(GetDatabase()); if (instance.CheckSecretsSeq()) { auto &context = *connection->context; diff --git a/src/pgduckdb_hooks.cpp b/src/pgduckdb_hooks.cpp index 184d2ffd..04137899 100644 --- a/src/pgduckdb_hooks.cpp +++ b/src/pgduckdb_hooks.cpp @@ -193,7 +193,8 @@ DuckdbPlannerHook_Unsafe(Query *parse, const char *query_string, int cursor_opti static PlannedStmt * DuckdbPlannerHook(Query *parse, const char *query_string, int cursor_options, ParamListInfo bound_params) { - return pgduckdb::DuckDBFunctionGuard(DuckdbPlannerHook_Unsafe, __FUNCTION__, parse, query_string, cursor_options, bound_params); + return pgduckdb::DuckDBFunctionGuard(DuckdbPlannerHook_Unsafe, __FUNCTION__, parse, query_string, + cursor_options, bound_params); } static void @@ -225,8 +226,8 @@ DuckdbUtilityHook_Unsafe(PlannedStmt *pstmt, const char *query_string, bool read static void DuckdbUtilityHook(PlannedStmt *pstmt, const char *query_string, bool read_only_tree, ProcessUtilityContext context, ParamListInfo params, struct QueryEnvironment *query_env, DestReceiver *dest, QueryCompletion *qc) { - pgduckdb::DuckDBFunctionGuard(DuckdbUtilityHook_Unsafe, __FUNCTION__, pstmt, query_string, read_only_tree, context, params, - query_env, dest, qc); + pgduckdb::DuckDBFunctionGuard(DuckdbUtilityHook_Unsafe, __FUNCTION__, pstmt, query_string, read_only_tree, + context, params, query_env, dest, qc); } extern "C" { diff --git a/src/pgduckdb_options.cpp b/src/pgduckdb_options.cpp index d7fe3cc3..a09f4e85 100644 --- a/src/pgduckdb_options.cpp +++ b/src/pgduckdb_options.cpp @@ -157,7 +157,8 @@ install_extension(PG_FUNCTION_ARGS) { PG_RETURN_BOOL(result); } -const char* pgduckdb_raw_query_unsafe(const char *query) { +const char * +pgduckdb_raw_query_unsafe(const char *query) { auto connection = pgduckdb::DuckDBManager::CreateConnection(); auto result = pgduckdb::DuckDBQueryOrThrow(*connection->context, query); return strdup(result->ToString().c_str()); diff --git a/src/pgduckdb_planner.cpp b/src/pgduckdb_planner.cpp index b335a04c..03a8c4c4 100644 --- a/src/pgduckdb_planner.cpp +++ b/src/pgduckdb_planner.cpp @@ -108,7 +108,7 @@ CreatePlan(Query *query) { PlannedStmt * DuckdbPlanNode(Query *parse, int cursor_options) { /* We need to check can we DuckDB create plan */ - Plan *plan = pgduckdb::DuckDBFunctionGuard(CreatePlan, "CreatePlan", parse); + Plan *plan = pgduckdb::DuckDBFunctionGuard(CreatePlan, "CreatePlan", parse); Plan *duckdb_plan = (Plan *)castNode(CustomScan, plan); if (!duckdb_plan) { diff --git a/src/pgduckdb_ruleutils.cpp b/src/pgduckdb_ruleutils.cpp index 3e751e20..9d458c56 100644 --- a/src/pgduckdb_ruleutils.cpp +++ b/src/pgduckdb_ruleutils.cpp @@ -1,3 +1,5 @@ +#include "duckdb.hpp" + extern "C" { #include "postgres.h" @@ -20,15 +22,6 @@ extern "C" { #include "pgduckdb/pgduckdb_metadata_cache.hpp" extern "C" { -char * -pgduckdb_function_name(Oid function_oid) { - if (!pgduckdb::IsDuckdbOnlyFunction(function_oid)) { - return nullptr; - } - auto func_name = get_func_name(function_oid); - return psprintf("system.main.%s", quote_identifier(func_name)); -} - /* * generate_relation_name computes the fully qualified name of the relation in * DuckDB for the specified Postgres OID. This includes the DuckDB database name @@ -36,10 +29,11 @@ pgduckdb_function_name(Oid function_oid) { */ char * pgduckdb_relation_name(Oid relation_oid) { - HeapTuple tp = SearchSysCache1(RELOID, ObjectIdGetDatum(relation_oid)); - if (!HeapTupleIsValid(tp)) - elog(ERROR, "cache lookup failed for relation %u", relation_oid); + if (!HeapTupleIsValid(tp)) { + throw std::runtime_error("cache lookup failed for relation '" + std::to_string(relation_oid) + "'"); + } + Form_pg_class relation = (Form_pg_class)GETSTRUCT(tp); const char *relname = NameStr(relation->relname); /* @@ -48,13 +42,7 @@ pgduckdb_relation_name(Oid relation_oid) { * of plain pg_temp. I don't think this really matters. */ const char *nspname = get_namespace_name_or_temp(relation->relnamespace); - const char *dbname; - - if (relation->relam == pgduckdb::DuckdbTableAmOid()) { - dbname = "memory"; - } else { - dbname = "pgduckdb"; - } + const char *dbname = relation->relam == pgduckdb::DuckdbTableAmOid() ? "memory" : "pgduckdb"; char *result = psprintf("%s.%s.%s", quote_identifier(dbname), quote_identifier(nspname), quote_identifier(relname)); @@ -85,7 +73,7 @@ pgduckdb_get_tabledef(Oid relation_oid) { initStringInfo(&buffer); if (get_rel_relkind(relation_oid) != RELKIND_RELATION) { - elog(ERROR, "Only regular tables are supported in DuckDB"); + throw duckdb::NotImplementedException("Only regular tables are supported in DuckDB"); } appendStringInfo(&buffer, "CREATE SCHEMA IF NOT EXISTS %s; ", quote_identifier(schema_name)); @@ -93,13 +81,13 @@ pgduckdb_get_tabledef(Oid relation_oid) { appendStringInfoString(&buffer, "CREATE "); if (relation->rd_rel->relpersistence != RELPERSISTENCE_TEMP) { - elog(ERROR, "Only TEMP tables are supported in DuckDB"); + throw duckdb::NotImplementedException("Only TEMP tables are supported in DuckDB"); } appendStringInfo(&buffer, "TABLE %s (", relation_name); if (list_length(RelationGetFKeyList(relation)) > 0) { - elog(ERROR, "DuckDB tables do not support foreign keys"); + throw duckdb::NotImplementedException("DuckDB tables do not support foreign keys"); } List *relation_context = pgduckdb_deparse_context_for(relation_name, relation_oid); @@ -135,11 +123,11 @@ pgduckdb_get_tabledef(Oid relation_oid) { appendStringInfoString(&buffer, column_type_name); if (column->attcompression) { - elog(ERROR, "Column compression is not supported in DuckDB"); + throw duckdb::NotImplementedException("Column compression is not supported in DuckDB"); } if (column->attidentity) { - elog(ERROR, "Identity columns are not supported in DuckDB"); + throw duckdb::NotImplementedException("Identity columns are not supported in DuckDB"); } /* if this column has a default value, append the default value */ @@ -171,9 +159,9 @@ pgduckdb_get_tabledef(Oid relation_oid) { if (!column->attgenerated) { appendStringInfo(&buffer, " DEFAULT %s", default_string); } else if (column->attgenerated == ATTRIBUTE_GENERATED_STORED) { - elog(ERROR, "DuckDB does not support STORED generated columns"); + throw duckdb::NotImplementedException("DuckDB does not support STORED generated columns"); } else { - elog(ERROR, "Unkown generated column type"); + throw std::runtime_error("Unkown generated column type"); } } @@ -191,7 +179,7 @@ pgduckdb_get_tabledef(Oid relation_oid) { Oid collation = column->attcollation; if (collation != InvalidOid && collation != DEFAULT_COLLATION_OID && collation != C_COLLATION_OID && collation != POSIX_COLLATION_OID) { - elog(ERROR, "DuckDB does not support column collations"); + throw duckdb::NotImplementedException("DuckDB does not support column collations"); } } @@ -229,12 +217,13 @@ pgduckdb_get_tabledef(Oid relation_oid) { if (!pgduckdb::IsDuckdbTableAm(relation->rd_tableam)) { /* Shouldn't happen but seems good to check anyway */ - elog(ERROR, "Only a table with the DuckDB can be stored in DuckDB, %d %d", relation->rd_rel->relam, - pgduckdb::DuckdbTableAmOid()); + throw std::runtime_error( + "Only a table with the DuckDB can be stored in DuckDB, (relam=" + std::to_string(relation->rd_rel->relam) + + ", oid=" + std::to_string(relation_oid) + ")"); } if (relation->rd_options) { - elog(ERROR, "Storage options are not supported in DuckDB"); + throw duckdb::NotImplementedException("Storage options are not supported in DuckDB"); } relation_close(relation, AccessShareLock);