From a1a79fde2fd627356fa026801efb6918cf72e155 Mon Sep 17 00:00:00 2001 From: Yves Date: Thu, 31 Oct 2024 18:25:39 +0100 Subject: [PATCH 1/4] Simplify `DuckDBQueryOrThrow` --- src/pgduckdb_utils.cpp | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/src/pgduckdb_utils.cpp b/src/pgduckdb_utils.cpp index 60a1915c..5055a425 100644 --- a/src/pgduckdb_utils.cpp +++ b/src/pgduckdb_utils.cpp @@ -63,21 +63,11 @@ CreateOrGetDirectoryPath(const char* directory_name) { duckdb::unique_ptr DuckDBQueryOrThrow(duckdb::ClientContext &context, const std::string &query) { - const char *error_message = nullptr; - { - auto res = context.Query(query, false); - if (!res->HasError()) { - return res; - } - - error_message = pstrdup(res->GetError().c_str()); + auto res = context.Query(query, false); + if (res->HasError()) { + res->ThrowError(); } - - if (error_message) { - elog(ERROR, "(PGDuckDB/DuckDBQuery) %s", error_message); - } - - return nullptr; // unreachable + return res; } duckdb::unique_ptr From 3c8a7ffb73ffb58a94e919c8e50a8d46c6389fb7 Mon Sep 17 00:00:00 2001 From: Yves Date: Thu, 31 Oct 2024 18:28:00 +0100 Subject: [PATCH 2/4] SPI_exec_or_throw --- include/pgduckdb/pgduckdb_utils.hpp | 2 ++ src/pgduckdb_ddl.cpp | 31 ++++++++++------------------- src/pgduckdb_utils.cpp | 9 +++++++++ 3 files changed, 21 insertions(+), 21 deletions(-) diff --git a/include/pgduckdb/pgduckdb_utils.hpp b/include/pgduckdb/pgduckdb_utils.hpp index 4a77b35f..2ba19722 100644 --- a/include/pgduckdb/pgduckdb_utils.hpp +++ b/include/pgduckdb/pgduckdb_utils.hpp @@ -82,6 +82,8 @@ __CPPFunctionGuard__(const char *func_name, FuncArgs... args) { #define InvokeCPPFunc(FUNC, ...) pgduckdb::__CPPFunctionGuard__(__FUNCTION__, __VA_ARGS__) +int SPI_exec_or_throw(const char *query, int tcount, int expected_ret_code); + // Wrappers #define DECLARE_PG_FUNCTION(func_name) \ diff --git a/src/pgduckdb_ddl.cpp b/src/pgduckdb_ddl.cpp index 5d91a6af..7ed3e8d7 100644 --- a/src/pgduckdb_ddl.cpp +++ b/src/pgduckdb_ddl.cpp @@ -162,7 +162,7 @@ DECLARE_PG_FUNCTION(duckdb_create_table_trigger) { SetConfigOption("search_path", "pg_catalog, pg_temp", PGC_USERSET, PGC_S_SESSION); SetConfigOption("duckdb.force_execution", "false", PGC_USERSET, PGC_S_SESSION); - int ret = SPI_exec(R"( + pgduckdb::SPI_exec_or_throw(R"( SELECT DISTINCT objid AS relid, pg_class.relpersistence = 't' AS is_temporary FROM pg_catalog.pg_event_trigger_ddl_commands() cmds JOIN pg_catalog.pg_class @@ -170,10 +170,7 @@ DECLARE_PG_FUNCTION(duckdb_create_table_trigger) { WHERE cmds.object_type = 'table' AND pg_class.relam = (SELECT oid FROM pg_am WHERE amname = 'duckdb') )", - 0); - - if (ret != SPI_OK_SELECT) - elog(ERROR, "SPI_exec failed: error code %s", SPI_result_code_string(ret)); + 0, SPI_OK_SELECT); /* if we selected a row it was a duckdb table */ auto is_duckdb_table = SPI_processed > 0; @@ -227,11 +224,11 @@ DECLARE_PG_FUNCTION(duckdb_create_table_trigger) { values[2] = CStringGetTextDatum(pgduckdb::current_motherduck_catalog_version); nulls[2] = ' '; } - ret = SPI_execute_with_args(R"( + int ret = SPI_execute_with_args(R"( INSERT INTO duckdb.tables (relid, duckdb_db, motherduck_catalog_version) VALUES ($1, $2, $3) )", - lengthof(arg_types), arg_types, values, nulls, false, 0); + lengthof(arg_types), arg_types, values, nulls, false, 0); /* Revert back to original privileges */ SetUserIdAndSecContext(saved_userid, sec_context); @@ -345,15 +342,13 @@ DECLARE_PG_FUNCTION(duckdb_drop_trigger) { SetConfigOption("duckdb.force_execution", "false", PGC_USERSET, PGC_S_SESSION); if (!pgduckdb::doing_motherduck_sync) { - int ret = SPI_exec(R"( + pgduckdb::SPI_exec_or_throw(R"( SELECT object_identity FROM pg_catalog.pg_event_trigger_dropped_objects() WHERE object_type = 'schema' AND object_identity LIKE 'ddb$%' )", - 0); - if (ret != SPI_OK_SELECT) - elog(ERROR, "SPI_exec failed: error code %s", SPI_result_code_string(ret)); + 0, SPI_OK_SELECT); if (SPI_processed > 0) { elog(ERROR, "Currently it's not possible to drop ddb$ schemas"); @@ -375,7 +370,7 @@ DECLARE_PG_FUNCTION(duckdb_drop_trigger) { * Here we first handle the non-temporary tables. */ - int ret = SPI_exec(R"( + pgduckdb::SPI_exec_or_throw(R"( DELETE FROM duckdb.tables USING ( SELECT objid, schema_name, object_name @@ -385,14 +380,11 @@ DECLARE_PG_FUNCTION(duckdb_drop_trigger) { WHERE relid = objid RETURNING objs.schema_name, objs.object_name )", - 0); + 0, SPI_OK_DELETE_RETURNING); /* Revert back to original privileges */ SetUserIdAndSecContext(saved_userid, sec_context); - if (ret != SPI_OK_DELETE_RETURNING) - elog(ERROR, "SPI_exec failed: error code %s", SPI_result_code_string(ret)); - /* * We lazily create a connection to DuckDB when we need it. That's * important because we don't want to impose our "prevent in transaction @@ -442,16 +434,13 @@ DECLARE_PG_FUNCTION(duckdb_drop_trigger) { * And now we basically do the same thing as above, but for TEMP tables. * For those we need to check our in-memory temporary_duckdb_tables set. */ - ret = SPI_exec(R"( + pgduckdb::SPI_exec_or_throw(R"( SELECT objid, object_name FROM pg_catalog.pg_event_trigger_dropped_objects() WHERE object_type = 'table' AND schema_name = 'pg_temp' )", - 0); - - if (ret != SPI_OK_SELECT) - elog(ERROR, "SPI_exec failed: error code %s", SPI_result_code_string(ret)); + 0, SPI_OK_SELECT); for (auto proc = 0; proc < SPI_processed; ++proc) { HeapTuple tuple = SPI_tuptable->vals[proc]; diff --git a/src/pgduckdb_utils.cpp b/src/pgduckdb_utils.cpp index 5055a425..6d52cef4 100644 --- a/src/pgduckdb_utils.cpp +++ b/src/pgduckdb_utils.cpp @@ -61,6 +61,15 @@ CreateOrGetDirectoryPath(const char* directory_name) { return duckdb_data_directory; } +int +SPI_exec_or_throw(const char *query, int tcount, int expected_ret_code) { + int ret = SPI_exec(query, tcount); + if (ret != expected_ret_code) { + throw std::runtime_error(std::string("SPI_exec failed: error code ") + SPI_result_code_string(ret)); + } + return ret; +} + duckdb::unique_ptr DuckDBQueryOrThrow(duckdb::ClientContext &context, const std::string &query) { auto res = context.Query(query, false); From 2daf4eeaa86b08c92fc46b3cdc4c8167ab902a6e Mon Sep 17 00:00:00 2001 From: Yves Date: Fri, 1 Nov 2024 08:44:28 +0100 Subject: [PATCH 3/4] Throw C++ exceptions --- include/pgduckdb/types/decimal.hpp | 4 +- src/catalog/pgduckdb_transaction.cpp | 2 +- src/pgduckdb_ddl.cpp | 40 ++++++++-------- src/pgduckdb_duckdb.cpp | 3 +- src/pgduckdb_options.cpp | 5 +- src/pgduckdb_ruleutils.cpp | 47 ++++++++++--------- src/pgduckdb_types.cpp | 3 +- src/utility/copy.cpp | 7 ++- test/regression/expected/non_superuser.out | 4 +- test/regression/expected/temporary_tables.out | 16 +++---- 10 files changed, 71 insertions(+), 60 deletions(-) diff --git a/include/pgduckdb/types/decimal.hpp b/include/pgduckdb/types/decimal.hpp index fa1af84f..317f5fc4 100644 --- a/include/pgduckdb/types/decimal.hpp +++ b/include/pgduckdb/types/decimal.hpp @@ -108,6 +108,8 @@ extern "C" { #include "utils/numeric.h" } +#include "duckdb/common/exception/conversion_exception.hpp" + typedef int16_t NumericDigit; struct NumericShort { @@ -258,7 +260,7 @@ CreateNumeric(const NumericVar &var, bool *have_error) { *have_error = true; return NULL; } else { - ereport(ERROR, (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE), errmsg("value overflows numeric format"))); + throw duckdb::ConversionException("value overflows numeric format"); } } return result; diff --git a/src/catalog/pgduckdb_transaction.cpp b/src/catalog/pgduckdb_transaction.cpp index e78ec39d..91e9b72e 100644 --- a/src/catalog/pgduckdb_transaction.cpp +++ b/src/catalog/pgduckdb_transaction.cpp @@ -60,7 +60,7 @@ SchemaItems::GetTable(const string &entry_name) { // Check if the Relation is a VIEW auto tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(rel_oid)); if (!HeapTupleIsValid(tuple)) { - elog(ERROR, "Cache lookup failed for relation %u", rel_oid); + throw std::runtime_error("Cache lookup failed for relation " + std::to_string(rel_oid)); } auto relForm = (Form_pg_class)GETSTRUCT(tuple); diff --git a/src/pgduckdb_ddl.cpp b/src/pgduckdb_ddl.cpp index 7ed3e8d7..cf040d94 100644 --- a/src/pgduckdb_ddl.cpp +++ b/src/pgduckdb_ddl.cpp @@ -75,7 +75,7 @@ DuckdbHandleDDL(Node *parsetree, const char *queryString) { } else if (IsA(parsetree, CreateSchemaStmt) && !pgduckdb::doing_motherduck_sync) { auto stmt = castNode(CreateSchemaStmt, parsetree); if (strncmp("ddb$", stmt->schemaname, 4) == 0) { - elog(ERROR, "Creating ddb$ schemas is currently not supported"); + throw duckdb::NotImplementedException("Creating ddb$ schemas is currently not supported"); } return; } else if (IsA(parsetree, RenameStmt)) { @@ -85,10 +85,10 @@ DuckdbHandleDDL(Node *parsetree, const char *queryString) { return; } if (strncmp("ddb$", stmt->subname, 4) == 0) { - elog(ERROR, "Changing the name of a ddb$ schema is currently not supported"); + throw duckdb::NotImplementedException("Changing the name of a ddb$ schema is currently not supported"); } if (strncmp("ddb$", stmt->newname, 4) == 0) { - elog(ERROR, "Changing a schema to a ddb$ schema is currently not supported"); + throw duckdb::NotImplementedException("Changing a schema to a ddb$ schema is currently not supported"); } return; } @@ -110,9 +110,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); } } @@ -182,19 +182,19 @@ DECLARE_PG_FUNCTION(duckdb_create_table_trigger) { } if (SPI_processed != 1) { - elog(ERROR, "Expected single table to be created, but found %" PRIu64, static_cast(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"); } Datum is_temporary_datum = SPI_getbinval(tuple, SPI_tuptable->tupdesc, 2, &isnull); if (isnull) { - elog(ERROR, "Expected temporary boolean to be returned, but found NULL"); + throw std::runtime_error("Expected temporary boolean to be returned, but found NULL"); } Oid relid = DatumGetObjectId(relid_datum); @@ -233,8 +233,9 @@ DECLARE_PG_FUNCTION(duckdb_create_table_trigger) { /* Revert back to original privileges */ SetUserIdAndSecContext(saved_userid, sec_context); - if (ret != SPI_OK_INSERT) - elog(ERROR, "SPI_exec failed: error code %s", SPI_result_code_string(ret)); + if (ret != SPI_OK_INSERT) { + throw std::runtime_error("SPI_exec failed: error code " + std::string(SPI_result_code_string(ret))); + } } AtEOXact_GUC(false, save_nestlevel); @@ -262,7 +263,7 @@ DECLARE_PG_FUNCTION(duckdb_create_table_trigger) { auto stmt = castNode(CreateTableAsStmt, parsetree); CheckOnCommitSupport(stmt->into->onCommit); } else { - elog(ERROR, "Unexpected parsetree type: %d", nodeTag(parsetree)); + throw std::runtime_error("Unexpected parsetree type: " + std::to_string(nodeTag(parsetree))); } /* @@ -351,7 +352,7 @@ DECLARE_PG_FUNCTION(duckdb_drop_trigger) { 0, SPI_OK_SELECT); if (SPI_processed > 0) { - elog(ERROR, "Currently it's not possible to drop ddb$ schemas"); + throw std::runtime_error("Currently it's not possible to drop ddb$ schemas"); } } @@ -448,7 +449,7 @@ DECLARE_PG_FUNCTION(duckdb_drop_trigger) { 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"); + std::runtime_error("Expected relid to be returned, but found NULL"); } Oid relid = DatumGetObjectId(relid_datum); if (temporary_duckdb_tables.count(relid) == 0) { @@ -545,8 +546,9 @@ DECLARE_PG_FUNCTION(duckdb_alter_table_trigger) { SetUserIdAndSecContext(saved_userid, sec_context); AtEOXact_GUC(false, save_nestlevel); - if (ret != SPI_OK_SELECT) - elog(ERROR, "SPI_exec failed: error code %s", SPI_result_code_string(ret)); + if (ret != SPI_OK_SELECT) { + 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_possibly_duckdb_table = SPI_processed > 0; @@ -561,11 +563,11 @@ DECLARE_PG_FUNCTION(duckdb_alter_table_trigger) { 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"); } Datum needs_to_check_temporary_set_datum = SPI_getbinval(tuple, SPI_tuptable->tupdesc, 2, &isnull); if (isnull) { - elog(ERROR, "Expected temporary boolean to be returned, but found NULL"); + throw std::runtime_error("Expected temporary boolean to be returned, but found NULL"); } Oid relid = DatumGetObjectId(relid_datum); @@ -588,7 +590,7 @@ DECLARE_PG_FUNCTION(duckdb_alter_table_trigger) { */ DECLARE_PG_FUNCTION(duckdb_grant_trigger) { 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"); if (!pgduckdb::IsExtensionRegistered()) { /* @@ -626,7 +628,7 @@ DECLARE_PG_FUNCTION(duckdb_grant_trigger) { Oid relation_oid = RangeVarGetRelid(object, AccessShareLock, false); Relation relation = RelationIdGetRelation(relation_oid); if (pgduckdb::IsMotherDuckTable(relation)) { - elog(ERROR, "MotherDuck tables do not support GRANT"); + throw std::runtime_error("MotherDuck tables do not support GRANT"); } RelationClose(relation); } diff --git a/src/pgduckdb_duckdb.cpp b/src/pgduckdb_duckdb.cpp index 34c36a14..9339b0ca 100644 --- a/src/pgduckdb_duckdb.cpp +++ b/src/pgduckdb_duckdb.cpp @@ -200,7 +200,8 @@ DuckDBManager::LoadExtensions(duckdb::ClientContext &context) { duckdb::unique_ptr DuckDBManager::CreateConnection() { if (!pgduckdb::IsDuckdbExecutionAllowed()) { - elog(ERROR, "DuckDB execution is not allowed because you have not been granted the duckdb.postgres_role"); + throw std::runtime_error( + "DuckDB execution is not allowed because you have not been granted the duckdb.postgres_role"); } auto &instance = Get(); diff --git a/src/pgduckdb_options.cpp b/src/pgduckdb_options.cpp index 54e4deeb..2240361f 100644 --- a/src/pgduckdb_options.cpp +++ b/src/pgduckdb_options.cpp @@ -131,8 +131,9 @@ DuckdbInstallExtension(Datum name_datum) { ON CONFLICT (name) DO UPDATE SET enabled = true )", lengthof(arg_types), arg_types, values, NULL, false, 0); - if (ret != SPI_OK_INSERT) - elog(ERROR, "SPI_exec failed: error code %s", SPI_result_code_string(ret)); + if (ret != SPI_OK_INSERT) { + throw std::runtime_error("SPI_exec failed: error code " + std::string(SPI_result_code_string(ret))); + } SPI_finish(); return true; diff --git a/src/pgduckdb_ruleutils.cpp b/src/pgduckdb_ruleutils.cpp index cbe9ec4f..dfd4160f 100644 --- a/src/pgduckdb_ruleutils.cpp +++ b/src/pgduckdb_ruleutils.cpp @@ -74,7 +74,7 @@ pgduckdb_db_and_schema(const char *postgres_schema_name, bool is_duckdb_table) { appendBinaryStringInfo(&db_name, saveptr, dollar - saveptr); saveptr = dollar + 1; if (saveptr[0] == '\0') { - elog(ERROR, "Schema name is invalid"); + throw std::runtime_error("Schema name is invalid"); } if (saveptr[0] == '$') { @@ -124,8 +124,10 @@ pgduckdb_db_and_schema_string(const char *postgres_schema_name, bool is_duckdb_t 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); const char *postgres_schema_name = get_namespace_name_or_temp(relation->relnamespace); @@ -144,10 +146,9 @@ pgduckdb_relation_name(Oid relation_oid) { * tables don't support RLS anyway. */ if (check_enable_rls(relation_oid, InvalidOid, false) == RLS_ENABLED) { - ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("(PGDuckDB/pgduckdb_relation_name) Cannot use \"%s\" in a DuckDB query, because RLS " - "is enabled on it", - get_rel_name(relation_oid)))); + throw duckdb::NotImplementedException("(PGDuckDB/pgduckdb_relation_name) Cannot use \"" + + std::string(relname) + + "\" in a DuckDB query, because RLS is enabled on it"); } } @@ -202,7 +203,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; ", db_and_schema); @@ -212,19 +213,22 @@ pgduckdb_get_tabledef(Oid relation_oid) { if (relation->rd_rel->relpersistence == RELPERSISTENCE_TEMP) { // allowed } else if (!pgduckdb::IsMotherDuckEnabledAnywhere()) { - elog(ERROR, "Only TEMP tables are supported in DuckDB if MotherDuck support is not enabled"); + throw duckdb::NotImplementedException( + "Only TEMP tables are supported in DuckDB if MotherDuck support is not enabled"); } else if (relation->rd_rel->relpersistence != RELPERSISTENCE_PERMANENT) { - elog(ERROR, "Only TEMP and non-UNLOGGED tables are supported in DuckDB"); + throw duckdb::NotImplementedException("Only TEMP and non-UNLOGGED tables are supported in DuckDB"); } else if (!pgduckdb::IsMotherDuckPostgresDatabase()) { - elog(ERROR, "MotherDuck tables must be created in the duckb.motherduck_postgres_database"); + throw duckdb::NotImplementedException( + "MotherDuck tables must be created in the duckb.motherduck_postgres_database"); } else if (relation->rd_rel->relowner != pgduckdb::MotherDuckPostgresUser()) { - elog(ERROR, "MotherDuck tables must be created by the duckb.motherduck_postgres_user"); + throw duckdb::NotImplementedException( + "MotherDuck tables must be created by the duckb.motherduck_postgres_user"); } 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); @@ -260,11 +264,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 */ @@ -296,9 +300,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"); } } @@ -316,7 +320,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"); } } @@ -354,12 +358,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); diff --git a/src/pgduckdb_types.cpp b/src/pgduckdb_types.cpp index 0563dcd2..5dd5fe84 100644 --- a/src/pgduckdb_types.cpp +++ b/src/pgduckdb_types.cpp @@ -782,7 +782,8 @@ ConvertPostgresParameterToDuckValue(Datum value, Oid postgres_type) { case FLOAT8OID: return duckdb::Value::DOUBLE(DatumGetFloat8(value)); default: - elog(ERROR, "Could not convert Postgres parameter of type: %d to DuckDB type", postgres_type); + throw duckdb::NotImplementedException("Could not convert Postgres parameter of type: %d to DuckDB type", + postgres_type); } } diff --git a/src/utility/copy.cpp b/src/utility/copy.cpp index dc1f7f47..c68aecf4 100644 --- a/src/utility/copy.cpp +++ b/src/utility/copy.cpp @@ -110,10 +110,9 @@ CheckQueryPermissions(Query *query, const char *query_string) { foreach_node(RangeTblEntry, rte, postgres_plan->rtable) { if (check_enable_rls(rte->relid, InvalidOid, false) == RLS_ENABLED) { - ereport(ERROR, - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("(PGDuckDB/CheckQueryPermissions) RLS enabled on \"%s\", cannot use DuckDB based COPY", - get_rel_name(rte->relid)))); + throw duckdb::NotImplementedException("(PGDuckDB/CheckQueryPermissions) RLS enabled on \"" + + std::string(get_rel_name(rte->relid)) + + "\", cannot use DuckDB based COPY"); } } } diff --git a/test/regression/expected/non_superuser.out b/test/regression/expected/non_superuser.out index 37574149..6fb60f57 100644 --- a/test/regression/expected/non_superuser.out +++ b/test/regression/expected/non_superuser.out @@ -38,7 +38,7 @@ ERROR: (PGDuckDB/CreatePlan) Prepared query returned an error: 'Permission Erro -- Should fail because DuckDB execution is not allowed for this user SET ROLE user3; SELECT * FROM t; -ERROR: DuckDB execution is not allowed because you have not been granted the duckdb.postgres_role +ERROR: (PGDuckDB/DuckdbPlanNode) DuckDB execution is not allowed because you have not been granted the duckdb.postgres_role -- Should work with regular posgres execution though, because this user is -- allowed to read the table. SET duckdb.force_execution = false; @@ -60,7 +60,7 @@ SELECT * FROM t; -- Should fail now, we don't support RLS SET ROLE user1; SELECT * FROM t; -ERROR: (PGDuckDB/pgduckdb_relation_name) Cannot use "t" in a DuckDB query, because RLS is enabled on it +ERROR: (PGDuckDB/DuckdbPlanNode) Not implemented Error: (PGDuckDB/pgduckdb_relation_name) Cannot use "t" in a DuckDB query, because RLS is enabled on it RESET ROLE; DROP TABLE t; DROP USER user1, user2; diff --git a/test/regression/expected/temporary_tables.out b/test/regression/expected/temporary_tables.out index b5b57889..d4c14770 100644 --- a/test/regression/expected/temporary_tables.out +++ b/test/regression/expected/temporary_tables.out @@ -132,7 +132,7 @@ VARCHAR VARCHAR VARCHAR (1 row) CREATE TABLE t(a int); -ERROR: Only TEMP tables are supported in DuckDB if MotherDuck support is not enabled +ERROR: (PGDuckDB/duckdb_create_table_trigger) Not implemented Error: Only TEMP tables are supported in DuckDB if MotherDuck support is not enabled -- XXX: A better error message would be nice here, but for now this is acceptable. CREATE TEMP TABLE t(a int PRIMARY KEY); ERROR: duckdb does not implement duckdb_index_build_range_scan @@ -140,12 +140,12 @@ ERROR: duckdb does not implement duckdb_index_build_range_scan CREATE TEMP TABLE t(a int UNIQUE); ERROR: duckdb does not implement duckdb_index_build_range_scan CREATE TEMP TABLE t(a int, b int GENERATED ALWAYS AS (a + 1) STORED); -ERROR: DuckDB does not support STORED generated columns +ERROR: (PGDuckDB/duckdb_create_table_trigger) Not implemented Error: DuckDB does not support STORED generated columns CREATE TEMP TABLE t(a int GENERATED ALWAYS AS IDENTITY); -ERROR: Identity columns are not supported in DuckDB +ERROR: (PGDuckDB/duckdb_create_table_trigger) Not implemented Error: Identity columns are not supported in DuckDB CREATE TEMP TABLE theap(b int PRIMARY KEY) USING heap; CREATE TEMP TABLE t(a int REFERENCES theap(b)); -ERROR: DuckDB tables do not support foreign keys +ERROR: (PGDuckDB/duckdb_create_table_trigger) Not implemented Error: DuckDB tables do not support foreign keys DROP TABLE theap; -- allowed but all other collations are not supported CREATE TEMP TABLE t(a text COLLATE "default"); @@ -155,11 +155,11 @@ DROP TABLE t; CREATE TEMP TABLE t(a text COLLATE "POSIX"); DROP TABLE t; CREATE TEMP TABLE t(a text COLLATE "de-x-icu"); -ERROR: DuckDB does not support column collations +ERROR: (PGDuckDB/duckdb_create_table_trigger) Not implemented Error: DuckDB does not support column collations CREATE TEMP TABLE t(A text COMPRESSION "pglz"); -ERROR: Column compression is not supported in DuckDB +ERROR: (PGDuckDB/duckdb_create_table_trigger) Not implemented Error: Column compression is not supported in DuckDB CREATE TEMP TABLE t(a int) WITH (fillfactor = 50); -ERROR: Storage options are not supported in DuckDB +ERROR: (PGDuckDB/duckdb_create_table_trigger) Not implemented Error: Storage options are not supported in DuckDB CREATE TEMP TABLE cities_duckdb ( name text, population real, @@ -205,7 +205,7 @@ SELECT * FROM t; DROP TABLE t; -- unsupported CREATE TEMP TABLE t(a int) ON COMMIT DROP; -ERROR: DuckDB does not support ON COMMIT DROP +ERROR: (PGDuckDB/duckdb_create_table_trigger) Not implemented Error: DuckDB does not support ON COMMIT DROP -- CTAS fully in Duckdb CREATE TEMP TABLE webpages USING duckdb AS SELECT * FROM read_csv('../../data/web_page.csv') as (column00 int, column01 text, column02 date); SELECT * FROM webpages ORDER BY column00 LIMIT 2; From ad0a63c697dd254383764365b3720b0e13852e00 Mon Sep 17 00:00:00 2001 From: Yves Date: Fri, 1 Nov 2024 08:44:49 +0100 Subject: [PATCH 4/4] C++ version --- src/pgduckdb_duckdb.cpp | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/src/pgduckdb_duckdb.cpp b/src/pgduckdb_duckdb.cpp index 9339b0ca..06ee9ea9 100644 --- a/src/pgduckdb_duckdb.cpp +++ b/src/pgduckdb_duckdb.cpp @@ -132,30 +132,28 @@ DuckDBManager::LoadSecrets(duckdb::ClientContext &context) { int secret_id = 0; for (auto &secret : duckdb_secrets) { - StringInfo secret_key = makeStringInfo(); + std::ostringstream query; bool is_r2_cloud_secret = (secret.type.rfind("R2", 0) == 0); - appendStringInfo(secret_key, "CREATE SECRET pgduckb_secret_%d ", secret_id); - appendStringInfo(secret_key, "(TYPE %s, KEY_ID '%s', SECRET '%s'", secret.type.c_str(), secret.key_id.c_str(), - secret.secret.c_str()); + query << "CREATE SECRET pgduckb_secret_" << secret_id << " "; + query << "(TYPE " << secret.type << ", KEY_ID '" << secret.key_id << "', SECRET '" << secret.secret << "'"; if (secret.region.length() && !is_r2_cloud_secret) { - appendStringInfo(secret_key, ", REGION '%s'", secret.region.c_str()); + query << ", REGION '" << secret.region << "'"; } if (secret.session_token.length() && !is_r2_cloud_secret) { - appendStringInfo(secret_key, ", SESSION_TOKEN '%s'", secret.session_token.c_str()); + query << ", SESSION_TOKEN '" << secret.session_token << "'"; } if (secret.endpoint.length() && !is_r2_cloud_secret) { - appendStringInfo(secret_key, ", ENDPOINT '%s'", secret.endpoint.c_str()); + query << ", ENDPOINT '" << secret.endpoint << "'"; } if (is_r2_cloud_secret) { - appendStringInfo(secret_key, ", ACCOUNT_ID '%s'", secret.endpoint.c_str()); + query << ", ACCOUNT_ID '" << secret.endpoint << "'"; } if (!secret.use_ssl) { - appendStringInfo(secret_key, ", USE_SSL 'FALSE'"); + query << ", USE_SSL 'FALSE'"; } - appendStringInfo(secret_key, ");"); - DuckDBQueryOrThrow(context, secret_key->data); + query << ");"; - pfree(secret_key->data); + DuckDBQueryOrThrow(context, query.str()); secret_id++; secret_table_num_rows = secret_id; } @@ -163,7 +161,7 @@ DuckDBManager::LoadSecrets(duckdb::ClientContext &context) { void DuckDBManager::DropSecrets(duckdb::ClientContext &context) { - for (auto secret_id = 0; secret_id < secret_table_num_rows; secret_id++) { + 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); pgduckdb::DuckDBQueryOrThrow(context, drop_secret_cmd); }