diff --git a/.gitmodules b/.gitmodules index cee8d314..aac2855e 100644 --- a/.gitmodules +++ b/.gitmodules @@ -1,4 +1,3 @@ [submodule "third_party/duckdb"] path = third_party/duckdb url = https://github.com/duckdb/duckdb.git - hash = 17d598fc4472c64969f47f86c30fce75c4d64ed4 diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 47f16ae0..afb594bc 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -128,6 +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. +* 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. diff --git a/include/pgduckdb/pgduckdb_types.hpp b/include/pgduckdb/pgduckdb_types.hpp index 004c7c39..2f0d00a3 100644 --- a/include/pgduckdb/pgduckdb_types.hpp +++ b/include/pgduckdb/pgduckdb_types.hpp @@ -20,7 +20,7 @@ constexpr int64_t PGDUCKDB_DUCK_TIMESTAMP_OFFSET = INT64CONST(10957) * USECS_PER duckdb::LogicalType ConvertPostgresToDuckColumnType(Form_pg_attribute &attribute); Oid GetPostgresDuckDBType(duckdb::LogicalType type); void ConvertPostgresToDuckValue(Datum value, duckdb::Vector &result, idx_t offset); -void ConvertDuckToPostgresValue(TupleTableSlot *slot, duckdb::Value &value, idx_t col); +bool ConvertDuckToPostgresValue(TupleTableSlot *slot, duckdb::Value &value, idx_t col); void InsertTupleIntoChunk(duckdb::DataChunk &output, duckdb::shared_ptr scan_global_state, duckdb::shared_ptr scan_local_state, HeapTupleData *tuple); diff --git a/include/pgduckdb/pgduckdb_utils.hpp b/include/pgduckdb/pgduckdb_utils.hpp index 3e16c6cb..e1ac4abb 100644 --- a/include/pgduckdb/pgduckdb_utils.hpp +++ b/include/pgduckdb/pgduckdb_utils.hpp @@ -1,11 +1,15 @@ #pragma once +extern "C" { +#include "postgres.h" +} + +#include "duckdb/common/exception.hpp" + #include #include #include -#include - namespace pgduckdb { inline std::vector @@ -19,4 +23,59 @@ TokenizeString(char *str, const char delimiter) { return v; }; +/* + * DuckdbGlobalLock should be held before calling. + */ +template +T +PostgresFunctionGuard(FuncType postgres_function, FuncArgs... args) { + T return_value; + bool error = false; + MemoryContext ctx = CurrentMemoryContext; + ErrorData *edata = nullptr; + // clang-format off + PG_TRY(); + { + return_value = postgres_function(args...); + } + PG_CATCH(); + { + MemoryContextSwitchTo(ctx); + edata = CopyErrorData(); + FlushErrorState(); + error = true; + } + PG_END_TRY(); + // clang-format on + if (error) { + throw duckdb::Exception(duckdb::ExceptionType::EXECUTOR, edata->message); + } + return return_value; +} + +template +void +PostgresFunctionGuard(FuncType postgres_function, FuncArgs... args) { + bool error = false; + MemoryContext ctx = CurrentMemoryContext; + ErrorData *edata = nullptr; + // clang-format off + PG_TRY(); + { + postgres_function(args...); + } + PG_CATCH(); + { + MemoryContextSwitchTo(ctx); + edata = CopyErrorData(); + FlushErrorState(); + error = true; + } + PG_END_TRY(); + // clang-format on + if (error) { + throw duckdb::Exception(duckdb::ExceptionType::EXECUTOR, edata->message); + } +} + } // namespace pgduckdb diff --git a/include/pgduckdb/types/decimal.hpp b/include/pgduckdb/types/decimal.hpp index f9ca8568..fa1af84f 100644 --- a/include/pgduckdb/types/decimal.hpp +++ b/include/pgduckdb/types/decimal.hpp @@ -199,8 +199,11 @@ CreateNumeric(const NumericVar &var, bool *have_error) { * but it seems worthwhile to expend a few cycles to ensure that we * never write any nonzero reserved bits to disk. */ - if (!(sign == NUMERIC_NAN || sign == NUMERIC_PINF || sign == NUMERIC_NINF)) - elog(ERROR, "invalid numeric sign value 0x%x", sign); + if (!(sign == NUMERIC_NAN || sign == NUMERIC_PINF || sign == NUMERIC_NINF)) { + elog(WARNING, "(PGDuckdDB/CreateNumeric) Invalid numeric sign value 0x%x", sign); + *have_error = true; + return NULL; + } result = (Numeric)palloc(NUMERIC_HDRSZ_SHORT); diff --git a/src/pgduckdb.cpp b/src/pgduckdb.cpp index f7a829ab..ce972606 100644 --- a/src/pgduckdb.cpp +++ b/src/pgduckdb.cpp @@ -5,7 +5,6 @@ extern "C" { #include "pgduckdb/pgduckdb.h" #include "pgduckdb/pgduckdb_node.hpp" -#include "pgduckdb/pgduckdb_utils.hpp" static void DuckdbInitGUC(void); diff --git a/src/pgduckdb_detoast.cpp b/src/pgduckdb_detoast.cpp index 4f06c737..665b7640 100644 --- a/src/pgduckdb_detoast.cpp +++ b/src/pgduckdb_detoast.cpp @@ -22,6 +22,7 @@ extern "C" { #include "pgduckdb/pgduckdb_process_lock.hpp" #include "pgduckdb/pgduckdb_types.hpp" #include "pgduckdb/pgduckdb_detoast.hpp" +#include "pgduckdb/pgduckdb_utils.hpp" /* * Following functions are direct logic found in postgres code but for duckdb execution they are needed to be thread @@ -41,8 +42,9 @@ PglzDecompressDatum(const struct varlena *value) { raw_size = pglz_decompress((char *)value + VARHDRSZ_COMPRESSED, VARSIZE(value) - VARHDRSZ_COMPRESSED, VARDATA(result), VARDATA_COMPRESSED_GET_EXTSIZE(value), true); - if (raw_size < 0) - ereport(ERROR, (errcode(ERRCODE_DATA_CORRUPTED), errmsg_internal("compressed pglz data is corrupt"))); + if (raw_size < 0) { + throw duckdb::InvalidInputException("(PGDuckDB/PglzDecompressDatum) Compressed pglz data is corrupt"); + } SET_VARSIZE(result, raw_size + VARHDRSZ); @@ -61,8 +63,9 @@ Lz4DecompresDatum(const struct varlena *value) { raw_size = LZ4_decompress_safe((char *)value + VARHDRSZ_COMPRESSED, VARDATA(result), VARSIZE(value) - VARHDRSZ_COMPRESSED, VARDATA_COMPRESSED_GET_EXTSIZE(value)); - if (raw_size < 0) - ereport(ERROR, (errcode(ERRCODE_DATA_CORRUPTED), errmsg_internal("compressed lz4 data is corrupt"))); + if (raw_size < 0) { + throw duckdb::InvalidInputException("(PGDuckDB/Lz4DecompresDatum) Compressed lz4 data is corrupt"); + } SET_VARSIZE(result, raw_size + VARHDRSZ); @@ -80,20 +83,22 @@ ToastDecompressDatum(struct varlena *attr) { case TOAST_LZ4_COMPRESSION_ID: return Lz4DecompresDatum(attr); default: - elog(ERROR, "invalid compression method id %d", TOAST_COMPRESS_METHOD(attr)); + throw duckdb::InvalidInputException("(PGDuckDB/ToastDecompressDatum) Invalid compression method id %d", + TOAST_COMPRESS_METHOD(attr)); return NULL; /* keep compiler quiet */ } } static struct varlena * ToastFetchDatum(struct varlena *attr) { - Relation toastrel; + Relation toast_rel; struct varlena *result; struct varatt_external toast_pointer; int32 attrsize; - if (!VARATT_IS_EXTERNAL_ONDISK(attr)) - elog(ERROR, "toast_fetch_datum shouldn't be called for non-ondisk datums"); + if (!VARATT_IS_EXTERNAL_ONDISK(attr)) { + throw duckdb::InvalidInputException("(PGDuckDB/ToastFetchDatum) Shouldn't be called for non-ondisk datums"); + } /* Must copy to access aligned fields */ VARATT_EXTERNAL_GET_POINTER(toast_pointer, attr); @@ -108,14 +113,22 @@ ToastFetchDatum(struct varlena *attr) { SET_VARSIZE(result, attrsize + VARHDRSZ); } - if (attrsize == 0) + if (attrsize == 0) { return result; + } + + std::lock_guard lock(DuckdbProcessLock::GetLock()); + + toast_rel = PostgresFunctionGuard(try_table_open, toast_pointer.va_toastrelid, AccessShareLock); + + if (toast_rel == NULL) { + throw duckdb::InternalException("(PGDuckDB/ToastFetchDatum) Error toast relation is NULL"); + } + + PostgresFunctionGuard(table_relation_fetch_toast_slice, toast_rel, toast_pointer.va_valueid, attrsize, 0, attrsize, + result); - DuckdbProcessLock::GetLock().lock(); - toastrel = table_open(toast_pointer.va_toastrelid, AccessShareLock); - table_relation_fetch_toast_slice(toastrel, toast_pointer.va_valueid, attrsize, 0, attrsize, result); - table_close(toastrel, AccessShareLock); - DuckdbProcessLock::GetLock().unlock(); + PostgresFunctionGuard(table_close, toast_rel, AccessShareLock); return result; } diff --git a/src/pgduckdb_duckdb.cpp b/src/pgduckdb_duckdb.cpp index 115046d3..26f6659a 100644 --- a/src/pgduckdb_duckdb.cpp +++ b/src/pgduckdb_duckdb.cpp @@ -24,21 +24,21 @@ CheckDataDirectory(const char *data_directory) { if (lstat(data_directory, &info) != 0) { if (errno == ENOENT) { - elog(DEBUG2, "Directory `%s` doesn't exists.", data_directory); + elog(DEBUG2, "(PGDuckDB/CheckDataDirectory) Directory `%s` doesn't exists", data_directory); return false; } else if (errno == EACCES) { - elog(ERROR, "Can't access `%s` directory.", data_directory); + elog(ERROR, "(PGDuckDB/CheckDataDirectory) Can't access `%s` directory", data_directory); } else { - elog(ERROR, "Other error when reading `%s`.", data_directory); + elog(ERROR, "(PGDuckDB/CheckDataDirectory) Other error when reading `%s`", data_directory); } } if (!S_ISDIR(info.st_mode)) { - elog(WARNING, "`%s` is not directory.", data_directory); + elog(WARNING, "(PGDuckDB/CheckDataDirectory) `%s` is not directory", data_directory); } if (access(data_directory, R_OK | W_OK)) { - elog(ERROR, "Directory `%s` permission problem.", data_directory); + elog(ERROR, "(PGDuckDB/CheckDataDirectory) Directory `%s` permission problem", data_directory); } return true; @@ -53,9 +53,12 @@ GetExtensionDirectory() { if (mkdir(duckdb_extension_data_directory->data, S_IRWXU | S_IRWXG | S_IRWXO) == -1) { int error = errno; pfree(duckdb_extension_data_directory->data); - elog(ERROR, "Creating duckdb extensions directory failed with reason `%s`\n", strerror(error)); + elog(ERROR, + "(PGDuckDB/GetExtensionDirectory) Creating duckdb extensions directory failed with reason `%s`\n", + strerror(error)); } - elog(DEBUG2, "Created %s as `duckdb.data_dir`", duckdb_extension_data_directory->data); + elog(DEBUG2, "(PGDuckDB/GetExtensionDirectory) Created %s as `duckdb.data_dir`", + duckdb_extension_data_directory->data); }; std::string duckdb_extension_directory(duckdb_extension_data_directory->data); @@ -64,7 +67,7 @@ GetExtensionDirectory() { } DuckDBManager::DuckDBManager() { - elog(DEBUG2, "Creating DuckDB instance"); + elog(DEBUG2, "(PGDuckDB/DuckDBManager) Creating DuckDB instance"); duckdb::DBConfig config; config.SetOptionByName("extension_directory", GetExtensionDirectory()); @@ -139,7 +142,7 @@ DuckDBManager::LoadExtensions(duckdb::ClientContext &context) { appendStringInfo(duckdb_extension, "LOAD %s;", extension.name.c_str()); auto res = context.Query(duckdb_extension->data, false); if (res->HasError()) { - elog(ERROR, "Extension `%s` could not be loaded with DuckDB", extension.name.c_str()); + elog(ERROR, "(PGDuckDB/LoadExtensions) `%s` could not be loaded with DuckDB", extension.name.c_str()); } } pfree(duckdb_extension->data); diff --git a/src/pgduckdb_filter.cpp b/src/pgduckdb_filter.cpp index 7e3f0752..200e3b42 100644 --- a/src/pgduckdb_filter.cpp +++ b/src/pgduckdb_filter.cpp @@ -70,7 +70,8 @@ FilterOperationSwitch(Datum &value, duckdb::Value &constant, Oid type_oid) { case VARCHAROID: return StringFilterOperation(value, constant); default: - elog(ERROR, "(DuckDB/FilterOperationSwitch) Unsupported duckdb type: %d", type_oid); + throw duckdb::InvalidTypeException( + duckdb::string("(DuckDB/FilterOperationSwitch) Unsupported duckdb type: %d", type_oid)); } } diff --git a/src/pgduckdb_hooks.cpp b/src/pgduckdb_hooks.cpp index 8dacf49c..6c7c6d70 100644 --- a/src/pgduckdb_hooks.cpp +++ b/src/pgduckdb_hooks.cpp @@ -127,7 +127,7 @@ DuckdbPlannerHook(Query *parse, const char *query_string, int cursor_options, Pa if (NeedsDuckdbExecution(parse)) { if (!IsAllowedStatement(parse)) { - elog(ERROR, "only SELECT statements involving DuckDB are supported"); + elog(ERROR, "(PGDuckDB/DuckdbPlannerHook) Only SELECT statements involving DuckDB are supported."); } PlannedStmt *duckdbPlan = DuckdbPlanNode(parse, cursor_options, bound_params); if (duckdbPlan) { diff --git a/src/pgduckdb_metadata_cache.cpp b/src/pgduckdb_metadata_cache.cpp index 43edc4eb..8031cdf4 100644 --- a/src/pgduckdb_metadata_cache.cpp +++ b/src/pgduckdb_metadata_cache.cpp @@ -62,7 +62,10 @@ InvalidateCaches(Datum arg, int cache_id, uint32 hash_value) { return; } cache.valid = false; - list_free(cache.duckdb_only_functions); + if (cache.installed) { + list_free(cache.duckdb_only_functions); + cache.duckdb_only_functions = NIL; + } } /* diff --git a/src/pgduckdb_node.cpp b/src/pgduckdb_node.cpp index b4baa09a..daf267ed 100644 --- a/src/pgduckdb_node.cpp +++ b/src/pgduckdb_node.cpp @@ -77,7 +77,6 @@ ExecuteQuery(DuckdbScanState *state) { auto &executor = duckdb::Executor::Get(*connection->context); // Wait for all tasks to terminate executor.CancelTasks(); - // Delete the scan state CleanupDuckdbScanState(state); // Process the interrupt on the Postgres side @@ -86,7 +85,8 @@ ExecuteQuery(DuckdbScanState *state) { } } while (!duckdb::PendingQueryResult::IsResultReady(execution_result)); if (execution_result == duckdb::PendingExecutionResult::EXECUTION_ERROR) { - elog(ERROR, "Duckdb execute returned an error: %s", pending->GetError().c_str()); + CleanupDuckdbScanState(state); + elog(ERROR, "(PGDuckDB/ExecuteQuery) %s", pending->GetError().c_str()); } query_results = pending->Execute(); state->column_count = query_results->ColumnCount(); @@ -127,7 +127,10 @@ Duckdb_ExecCustomScan(CustomScanState *node) { slot->tts_isnull[col] = true; } else { slot->tts_isnull[col] = false; - pgduckdb::ConvertDuckToPostgresValue(slot, value, col); + if (!pgduckdb::ConvertDuckToPostgresValue(slot, value, col)) { + CleanupDuckdbScanState(duckdb_scan_state); + elog(ERROR, "(PGDuckDB/Duckdb_ExecCustomScan) Value conversion failed"); + } } } diff --git a/src/pgduckdb_options.cpp b/src/pgduckdb_options.cpp index 11ae391a..2c703726 100644 --- a/src/pgduckdb_options.cpp +++ b/src/pgduckdb_options.cpp @@ -128,7 +128,7 @@ DuckdbInstallExtension(Datum name) { pfree(install_extension_command->data); if (res->HasError()) { - elog(WARNING, "(duckdb_install_extension) %s", res->GetError().c_str()); + elog(WARNING, "(PGDuckDB/DuckdbInstallExtension) %s", res->GetError().c_str()); return false; } diff --git a/src/pgduckdb_planner.cpp b/src/pgduckdb_planner.cpp index 2a64a491..05cadb64 100644 --- a/src/pgduckdb_planner.cpp +++ b/src/pgduckdb_planner.cpp @@ -70,7 +70,8 @@ CreatePlan(Query *query, const char *query_string, ParamListInfo bound_params) { auto prepared_query = context->Prepare(query_string); if (prepared_query->HasError()) { - elog(WARNING, "(DuckDB) %s", prepared_query->GetError().c_str()); + elog(WARNING, "(PGDuckDB/CreatePlan) Prepared query returned an error: '%s", + prepared_query->GetError().c_str()); return nullptr; } @@ -82,12 +83,19 @@ CreatePlan(Query *query, const char *query_string, ParamListInfo bound_params) { auto &column = prepared_result_types[i]; Oid postgresColumnOid = pgduckdb::GetPostgresDuckDBType(column); + if (!OidIsValid(postgresColumnOid)) { + elog(WARNING, "(PGDuckDB/CreatePlan) Cache lookup failed for type %u", postgresColumnOid); + return nullptr; + } + HeapTuple tp; Form_pg_type typtup; tp = SearchSysCache1(TYPEOID, ObjectIdGetDatum(postgresColumnOid)); - if (!HeapTupleIsValid(tp)) - elog(ERROR, "cache lookup failed for type %u", postgresColumnOid); + if (!HeapTupleIsValid(tp)) { + elog(WARNING, "(PGDuckDB/CreatePlan) Cache lookup failed for type %u", postgresColumnOid); + return nullptr; + } typtup = (Form_pg_type)GETSTRUCT(tp); diff --git a/src/pgduckdb_types.cpp b/src/pgduckdb_types.cpp index a677e13f..7a8f4048 100644 --- a/src/pgduckdb_types.cpp +++ b/src/pgduckdb_types.cpp @@ -321,7 +321,7 @@ ConvertDuckToPostgresArray(TupleTableSlot *slot, duckdb::Value &value, idx_t col slot->tts_values[col] = PointerGetDatum(arr); } -void +bool ConvertDuckToPostgresValue(TupleTableSlot *slot, duckdb::Value &value, idx_t col) { Oid oid = slot->tts_tupleDescriptor->attrs[col].atttypid; @@ -423,8 +423,8 @@ ConvertDuckToPostgresValue(TupleTableSlot *slot, duckdb::Value &value, idx_t col break; } default: { - throw duckdb::InternalException("Unrecognized physical type for DECIMAL value"); - break; + elog(WARNING, "(PGDuckDB/ConvertDuckToPostgresValue) Unrecognized physical type for DECIMAL value"); + return false; } } auto numeric = CreateNumeric(numeric_var, NULL); @@ -463,8 +463,10 @@ ConvertDuckToPostgresValue(TupleTableSlot *slot, duckdb::Value &value, idx_t col break; } default: - throw duckdb::NotImplementedException("(DuckDB/ConvertDuckToPostgresValue) Unsuported pgduckdb type: %d", oid); + elog(WARNING, "(PGDuckDB/ConvertDuckToPostgresValue) Unsuported pgduckdb type: %d", oid); + return false; } + return true; } static inline int @@ -599,7 +601,6 @@ GetPostgresDuckDBType(duckdb::LogicalType type) { duck_type = &child_type; } auto child_type_id = duck_type->id(); - switch (child_type_id) { case duckdb::LogicalTypeId::BOOLEAN: return BOOLARRAYOID; @@ -607,13 +608,17 @@ GetPostgresDuckDBType(duckdb::LogicalType type) { return INT4ARRAYOID; case duckdb::LogicalTypeId::BIGINT: return INT8ARRAYOID; - default: - throw duckdb::InvalidInputException("(DuckDB/GetPostgresDuckDBType) Unsupported pgduckdb type: %s", - type.ToString().c_str()); + default: { + elog(WARNING, "(PGDuckDB/GetPostgresDuckDBType) Unsupported `LIST` subtype %d to Postgres type", + (uint8)child_type_id); + return InvalidOid; + } } } default: { - elog(ERROR, "Could not convert DuckDB type: %s to Postgres type", type.ToString().c_str()); + elog(WARNING, "(PGDuckDB/GetPostgresDuckDBType) Could not convert DuckDB type: %s to Postgres type", + type.ToString().c_str()); + return InvalidOid; } } } diff --git a/src/scan/heap_reader.cpp b/src/scan/heap_reader.cpp index abdfd454..af2cc1f0 100644 --- a/src/scan/heap_reader.cpp +++ b/src/scan/heap_reader.cpp @@ -12,6 +12,9 @@ extern "C" { #include "pgduckdb/pgduckdb_process_lock.hpp" #include "pgduckdb/scan/heap_reader.hpp" #include "pgduckdb/pgduckdb_types.hpp" +#include "pgduckdb/pgduckdb_utils.hpp" + +#include namespace pgduckdb { @@ -84,11 +87,14 @@ HeapReader::ReadPageTuples(duckdb::DataChunk &output) { while (block != InvalidBlockNumber) { if (m_read_next_page) { CHECK_FOR_INTERRUPTS(); - DuckdbProcessLock::GetLock().lock(); + std::lock_guard lock(DuckdbProcessLock::GetLock()); block = m_block_number; - m_buffer = ReadBufferExtended(m_relation, MAIN_FORKNUM, block, RBM_NORMAL, GetAccessStrategy(BAS_BULKREAD)); - LockBuffer(m_buffer, BUFFER_LOCK_SHARE); - DuckdbProcessLock::GetLock().unlock(); + + m_buffer = PostgresFunctionGuard(ReadBufferExtended, m_relation, MAIN_FORKNUM, block, RBM_NORMAL, + GetAccessStrategy(BAS_BULKREAD)); + + PostgresFunctionGuard(LockBuffer, m_buffer, BUFFER_LOCK_SHARE); + page = PreparePageRead(); m_read_next_page = false; } diff --git a/src/scan/postgres_index_scan.cpp b/src/scan/postgres_index_scan.cpp index 5141e15d..f5f5d8ad 100644 --- a/src/scan/postgres_index_scan.cpp +++ b/src/scan/postgres_index_scan.cpp @@ -95,7 +95,8 @@ PostgresIndexScanFunction::PostgresIndexScanBind(duckdb::ClientContext &context, auto relation_descr = RelationGetDescr(rel); if (!relation_descr) { - elog(ERROR, "Failed to get tuple descriptor for relation with OID %u", rel->rd_id); + elog(WARNING, "(PGDuckDB/PostgresIndexScanBind) Failed to get tuple descriptor for relation with OID %u", + rel->rd_id); return nullptr; } @@ -106,7 +107,7 @@ PostgresIndexScanFunction::PostgresIndexScanBind(duckdb::ClientContext &context, return_types.push_back(duck_type); names.push_back(col_name); /* Log column name and type */ - elog(DEBUG3, "-- (DuckDB/PostgresHeapBind) Column name: %s, Type: %s --", col_name.c_str(), + elog(DEBUG2, "(PGDuckDB/PostgresIndexScanBind) Column name: %s, Type: %s --", col_name.c_str(), duck_type.ToString().c_str()); } diff --git a/src/scan/postgres_scan.cpp b/src/scan/postgres_scan.cpp index c7488ed1..29ed4eaa 100644 --- a/src/scan/postgres_scan.cpp +++ b/src/scan/postgres_scan.cpp @@ -119,18 +119,22 @@ ReplaceView(Oid view) { auto view_definiton = text_to_cstring(DatumGetTextP(view_def)); if (!view_definiton) { - elog(ERROR, "Could not retrieve view definition for Relation with relid: %u", view); + elog(WARNING, "(PGDuckDB/ReplaceView) Could not retrieve view definition for Relation with relid: %u", view); + return nullptr; } duckdb::Parser parser; parser.ParseQuery(view_definiton); auto statements = std::move(parser.statements); if (statements.size() != 1) { - elog(ERROR, "View definition contained more than 1 statement!"); + elog(WARNING, "(PGDuckDB/ReplaceView) View definition contained more than 1 statement!"); + return nullptr; } if (statements[0]->type != duckdb::StatementType::SELECT_STATEMENT) { - elog(ERROR, "View definition (%s) did not contain a SELECT statement!", view_definiton); + elog(WARNING, "(PGDuckDB/ReplaceView) View definition (%s) did not contain a SELECT statement!", + view_definiton); + return nullptr; } auto select = duckdb::unique_ptr_cast(std::move(statements[0])); @@ -182,7 +186,8 @@ PostgresReplacementScan(duckdb::ClientContext &context, duckdb::ReplacementScanI // Check if the Relation is a VIEW auto tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(relid)); if (!HeapTupleIsValid(tuple)) { - elog(ERROR, "Cache lookup failed for relation %u", relid); + elog(WARNING, "(PGDuckDB/PostgresReplacementScan) Cache lookup failed for relation %u", relid); + return nullptr; } auto relForm = (Form_pg_class)GETSTRUCT(tuple); diff --git a/src/scan/postgres_seq_scan.cpp b/src/scan/postgres_seq_scan.cpp index aefd83f2..f05859c8 100644 --- a/src/scan/postgres_seq_scan.cpp +++ b/src/scan/postgres_seq_scan.cpp @@ -14,7 +14,7 @@ PostgresSeqScanGlobalState::PostgresSeqScanGlobalState(Relation relation, duckdb m_heap_reader_global_state(duckdb::make_shared_ptr(relation)), m_relation(relation) { m_global_state->InitGlobalState(input); m_global_state->m_tuple_desc = RelationGetDescr(m_relation); - elog(DEBUG3, "-- (DuckDB/PostgresReplacementScanGlobalState) Running %lu threads -- ", MaxThreads()); + elog(DEBUG2, "(PGDuckDB/PostgresSeqScanGlobalState) Running %lu threads -- ", MaxThreads()); } PostgresSeqScanGlobalState::~PostgresSeqScanGlobalState() { @@ -76,7 +76,7 @@ PostgresSeqScanFunction::PostgresSeqScanBind(duckdb::ClientContext &context, duc auto relation_descr = RelationGetDescr(rel); if (!relation_descr) { - elog(ERROR, "Failed to get tuple descriptor for relation with OID %u", relid); + elog(WARNING, "(PGDuckDB/PostgresSeqScanBind) Failed to get tuple descriptor for relation with OID %u", relid); RelationClose(rel); return nullptr; } @@ -88,7 +88,7 @@ PostgresSeqScanFunction::PostgresSeqScanBind(duckdb::ClientContext &context, duc return_types.push_back(duck_type); names.push_back(col_name); /* Log column name and type */ - elog(DEBUG3, "-- (DuckDB/PostgresHeapBind) Column name: %s, Type: %s --", col_name.c_str(), + elog(DEBUG2, "(PGDuckDB/PostgresSeqScanBind) Column name: %s, Type: %s --", col_name.c_str(), duck_type.ToString().c_str()); } diff --git a/src/utility/copy.cpp b/src/utility/copy.cpp index 7a156e2a..836827d4 100644 --- a/src/utility/copy.cpp +++ b/src/utility/copy.cpp @@ -152,7 +152,7 @@ DuckdbCopy(PlannedStmt *pstmt, const char *query_string, struct QueryEnvironment auto res = duckdb_connection->context->Query(query_string, false); if (res->HasError()) { - elog(WARNING, "(Duckdb) %s", res->GetError().c_str()); + elog(WARNING, "(PGDuckDB/DuckdbCopy) Execution failed with an error: %s", res->GetError().c_str()); return false; } diff --git a/test/regression/expected/array_type_support.out b/test/regression/expected/array_type_support.out index b5d1c164..8ac36fd9 100644 --- a/test/regression/expected/array_type_support.out +++ b/test/regression/expected/array_type_support.out @@ -34,7 +34,7 @@ INSERT INTO int_array_2d VALUES ('{{11, 12, 13}, {14, 15, 16}}'), ('{{17, 18}, {19, 20}}'); SELECT * FROM int_array_2d; -ERROR: Duckdb execute returned an error: Invalid Input Error: Dimensionality of the schema and the data does not match, data contains more dimensions than the amount of dimensions specified by the schema +ERROR: (PGDuckDB/ExecuteQuery) Invalid Input Error: Dimensionality of the schema and the data does not match, data contains more dimensions than the amount of dimensions specified by the schema drop table int_array_2d; -- INT4 (single dimensional data, two dimensionsal type) CREATE TABLE int_array_2d(a INT[][]); @@ -44,7 +44,7 @@ INSERT INTO int_array_2d VALUES ('{11, 12, 13}'), ('{17, 18}'); SELECT * FROM int_array_2d; -ERROR: Duckdb execute returned an error: Invalid Input Error: Dimensionality of the schema and the data does not match, data contains fewer dimensions than the amount of dimensions specified by the schema +ERROR: (PGDuckDB/ExecuteQuery) Invalid Input Error: Dimensionality of the schema and the data does not match, data contains fewer dimensions than the amount of dimensions specified by the schema drop table int_array_2d; -- INT4 (two dimensional data and type) CREATE TABLE int_array_2d(a INT[][]); diff --git a/test/regression/expected/basic.out b/test/regression/expected/basic.out index 3bfa1c1a..fc407602 100644 --- a/test/regression/expected/basic.out +++ b/test/regression/expected/basic.out @@ -43,3 +43,7 @@ SET duckdb.max_threads_per_query TO default; SET client_min_messages TO default; DROP TABLE t; DROP TABLE empty; +-- Check that DROP / CREATE extension works +DROP EXTENSION pg_duckdb; +CREATE EXTENSION pg_duckdb; +WARNING: To actually execute queries using DuckDB you need to run "SET duckdb.execution TO true;" diff --git a/test/regression/expected/execution_error.out b/test/regression/expected/execution_error.out index c6ea6b66..d43a87c2 100644 --- a/test/regression/expected/execution_error.out +++ b/test/regression/expected/execution_error.out @@ -4,6 +4,6 @@ insert into int_as_varchar SELECT * from ( ('abc') ) t(a); select a::INTEGER from int_as_varchar; -ERROR: Duckdb execute returned an error: Conversion Error: Could not convert string 'abc' to INT32 +ERROR: (PGDuckDB/ExecuteQuery) Conversion Error: Could not convert string 'abc' to INT32 LINE 1: SELECT (a)::integer AS a FROM int_as_varchar ^ diff --git a/test/regression/expected/views.out b/test/regression/expected/views.out index 57ac4b1c..8c87c5cf 100644 --- a/test/regression/expected/views.out +++ b/test/regression/expected/views.out @@ -27,7 +27,7 @@ create table "s.t" as select 42; create view vw1 as select * from s.t; create view vw2 as select * from "s.t"; select * from vw1, vw2; -WARNING: (DuckDB) Binder Error: Referenced table "t" not found! +WARNING: (PGDuckDB/CreatePlan) Prepared query returned an error: 'Binder Error: Referenced table "t" not found! Candidate tables: "s.t" LINE 1: ...?column?", vw2."?column?" FROM (SELECT t."?column?" FROM s.t) vw1, (SELECT "s.... ^ diff --git a/test/regression/sql/basic.sql b/test/regression/sql/basic.sql index e8709e32..d32d3f0a 100644 --- a/test/regression/sql/basic.sql +++ b/test/regression/sql/basic.sql @@ -20,3 +20,8 @@ SET client_min_messages TO default; DROP TABLE t; DROP TABLE empty; + +-- Check that DROP / CREATE extension works + +DROP EXTENSION pg_duckdb; +CREATE EXTENSION pg_duckdb;