From d3f3bc8d67654ad32cd4d2d5db40f67cbd3bd983 Mon Sep 17 00:00:00 2001 From: Alex Kasko Date: Thu, 15 Oct 2026 13:18:09 +0100 Subject: [PATCH] Do not wrap postgres_query SQL unless necessary The query passed to `postgres_query()` function was being wrapped into a subquery with an explicit list of columns. This wrapping is necessary in some cases when a Postgres-side `CAST` is required for particular column. But it can be omitted in most of the usual cases. This PR enables the wrapping of the user query only when necessry. Ref: duckdblabs/duckdb-internal#11178 --- src/postgres_query.cpp | 4 +- src/postgres_scanner.cpp | 79 +++++++++++-------- test/sql/scanner/postgres_query.test | 114 ++++++++++++++++++++++++--- 3 files changed, 151 insertions(+), 46 deletions(-) diff --git a/src/postgres_query.cpp b/src/postgres_query.cpp index a1b531405..d28b23993 100644 --- a/src/postgres_query.cpp +++ b/src/postgres_query.cpp @@ -205,7 +205,9 @@ static unique_ptr PGQueryBind(ClientContext &context, TableFunctio auto converted_type = PostgresUtils::TypeToLogicalType(nullptr, nullptr, type_config, type_data, postgres_type); result->postgres_types.push_back(postgres_type); return_types.emplace_back(converted_type); - names.emplace_back(PQfname(describe_prepared, c)); + char *name_ptr = PQfname(describe_prepared, c); + string name = name_ptr ? name_ptr : StringUtil::Format("unnamed_%llu", c); + names.emplace_back(name); } int nparams = PQnparams(describe_prepared); if (nparams != param_values.size()) { diff --git a/src/postgres_scanner.cpp b/src/postgres_scanner.cpp index 3a98b832c..071698aa1 100644 --- a/src/postgres_scanner.cpp +++ b/src/postgres_scanner.cpp @@ -189,55 +189,64 @@ static bool ContainsCastToVarchar(const PostgresType &type) { return false; } -static void PostgresInitInternal(ClientContext &context, const PostgresBindData *bind_data_p, - PostgresLocalState &lstate, idx_t task_min, idx_t task_max) { - D_ASSERT(bind_data_p); - D_ASSERT(task_min <= task_max); - - auto bind_data = (const PostgresBindData *)bind_data_p; - +static string WriteColumnNames(const PostgresBindData &bind_data, const PostgresLocalState &lstate, + bool &col_names_required) { string col_names; + col_names_required = bind_data.names.size() != lstate.column_ids.size(); for (auto &column_id : lstate.column_ids) { if (!col_names.empty()) { col_names += ", "; } if (column_id == COLUMN_IDENTIFIER_ROW_ID) { - if (bind_data->table_name.empty() || !bind_data->emit_ctid) { + if (bind_data.table_name.empty() || !bind_data.emit_ctid) { // count(*) over postgres_query col_names += "NULL"; } else { col_names += "ctid"; } + col_names_required = true; } else { - col_names += PostgresUtils::WriteIdentifier(bind_data->names[column_id]); - if (bind_data->postgres_types[column_id].info == PostgresTypeAnnotation::CAST_TO_VARCHAR) { + col_names += PostgresUtils::WriteIdentifier(bind_data.names[column_id]); + if (bind_data.postgres_types[column_id].info == PostgresTypeAnnotation::CAST_TO_VARCHAR) { col_names += "::VARCHAR"; - } else if (bind_data->types[column_id].id() == LogicalTypeId::LIST) { - if (bind_data->postgres_types[column_id].info != PostgresTypeAnnotation::STANDARD) { + col_names_required = true; + } else if (bind_data.types[column_id].id() == LogicalTypeId::LIST) { + if (bind_data.postgres_types[column_id].info != PostgresTypeAnnotation::STANDARD) { continue; } - if (bind_data->postgres_types[column_id].children[0].info == PostgresTypeAnnotation::CAST_TO_VARCHAR) { + if (bind_data.postgres_types[column_id].children[0].info == PostgresTypeAnnotation::CAST_TO_VARCHAR) { col_names += "::VARCHAR[]"; + col_names_required = true; } } else { - if (ContainsCastToVarchar(bind_data->postgres_types[column_id])) { + if (ContainsCastToVarchar(bind_data.postgres_types[column_id])) { throw NotImplementedException("Error reading table \"%s\" - cast to varchar not implemented for " "composite column \"%s\" (type %s)", - bind_data->table_name, bind_data->names[column_id], - bind_data->types[column_id].ToString()); + bind_data.table_name, bind_data.names[column_id], + bind_data.types[column_id].ToString()); } } } } + return col_names; +} - string filter_string = - PostgresFilterPushdown::TransformFilters(lstate.column_ids, lstate.filters, bind_data->names); +static void PostgresInitInternal(ClientContext &context, const PostgresBindData *bind_data_p, + PostgresLocalState &lstate, idx_t task_min, idx_t task_max) { + D_ASSERT(bind_data_p); + D_ASSERT(task_min <= task_max); + + auto bind_data = bind_data_p->Cast(); + + bool col_names_required = false; + string col_names = WriteColumnNames(bind_data, lstate, col_names_required); + string filter_string = PostgresFilterPushdown::TransformFilters(lstate.column_ids, lstate.filters, bind_data.names); string filter; lstate.exec = false; lstate.done = false; - if (bind_data->pages_approx > 0) { + if (bind_data.pages_approx > 0) { filter = StringUtil::Format("WHERE ctid BETWEEN '(%d,0)'::tid AND '(%d,0)'::tid", task_min, task_max); } if (!filter_string.empty()) { @@ -248,24 +257,28 @@ static void PostgresInitInternal(ClientContext &context, const PostgresBindData } filter += filter_string; } - string query; - if (bind_data->table_name.empty()) { - D_ASSERT(!bind_data->sql.empty()); - query = - StringUtil::Format(R"(SELECT %s FROM (%s) AS __unnamed_subquery %s)", col_names, bind_data->sql, filter); + string query; + if (bind_data.table_name.empty()) { + D_ASSERT(!bind_data.sql.empty()); + if (col_names_required) { + query = + StringUtil::Format("SELECT %s FROM (%s) AS __unnamed_subquery %s", col_names, bind_data.sql, filter); + } else if (!filter.empty()) { + query = StringUtil::Format("%s %s", bind_data.sql, filter); + } else { + query = bind_data.sql; + } } else { - query = StringUtil::Format(R"(SELECT %s FROM %s.%s %s)", col_names, - PostgresUtils::WriteIdentifier(bind_data->schema_name), - PostgresUtils::WriteIdentifier(bind_data->table_name), filter); + query = StringUtil::Format("SELECT %s FROM %s.%s %s", col_names, + PostgresUtils::WriteIdentifier(bind_data.schema_name), + PostgresUtils::WriteIdentifier(bind_data.table_name), filter); } - if (!bind_data->limit_clause.empty()) { - query += bind_data->limit_clause; + if (!bind_data.limit_clause.empty()) { + query += bind_data.limit_clause; } - if (!(bind_data->use_text_protocol || bind_data->query_mode == PostgresQueryMode::PLAIN_QUERY_BINARY)) { - query = StringUtil::Format(R"(COPY (%s) TO STDOUT (FORMAT "binary");)", query); - } else { - query += ";"; + if (!(bind_data.use_text_protocol || bind_data.query_mode == PostgresQueryMode::PLAIN_QUERY_BINARY)) { + query = StringUtil::Format("COPY (%s) TO STDOUT (FORMAT \"binary\");", query); } lstate.sql = std::move(query); } diff --git a/test/sql/scanner/postgres_query.test b/test/sql/scanner/postgres_query.test index e2aca8041..46301ce25 100644 --- a/test/sql/scanner/postgres_query.test +++ b/test/sql/scanner/postgres_query.test @@ -10,7 +10,7 @@ statement ok ATTACH 'dbname=postgresscanner' AS s1 (TYPE POSTGRES) query III -select * from postgres_query('s1', 'SELECT * FROM cars'); +FROM postgres_query('s1', 'SELECT * FROM cars'); ---- ferari testarosa red aston martin db2 blue @@ -19,12 +19,12 @@ ford T black # filters query III -select * from postgres_query('s1', 'SELECT * FROM cars WHERE color=''red'''); +FROM postgres_query('s1', 'SELECT * FROM cars WHERE color=''red'''); ---- ferari testarosa red query III -select * from postgres_query('s1', 'SELECT * FROM cars') WHERE color='red'; +FROM postgres_query('s1', 'SELECT * FROM cars') WHERE color='red'; ---- ferari testarosa red @@ -77,24 +77,24 @@ DATE[] TIME[] TIME WITH TIME ZONE[] TIMESTAMP[] TIMESTAMP WITH TIME ZONE[] # semicolons query III -select * from postgres_query('s1', 'SELECT * FROM cars;') WHERE color='red'; +FROM postgres_query('s1', 'SELECT * FROM cars;') WHERE color='red'; ---- ferari testarosa red query III -select * from postgres_query('s1', 'SELECT * FROM cars; ; ') WHERE color='red'; +FROM postgres_query('s1', 'SELECT * FROM cars; ; ') WHERE color='red'; ---- ferari testarosa red # incorrect usage statement error -select * from postgres_query('s1', 'CREATE TABLE my_table(i INT)'); +FROM postgres_query('s1', 'CREATE TABLE my_table(i INT)'); ---- Invalid Error: Failed to execute query # database not found statement error -select * from postgres_query('xx', 'SELECT * FROM cars'); +FROM postgres_query('xx', 'SELECT * FROM cars'); ---- Binder Error: Failed to find attached database @@ -102,12 +102,12 @@ mode skip # empty statements statement error -select * from postgres_query('s1', '') WHERE color='red'; +FROM postgres_query('s1', '') WHERE color='red'; ---- Invalid Error: Failed to execute query statement error -select * from postgres_query('s1', ';;') WHERE color='red'; +FROM postgres_query('s1', ';;') WHERE color='red'; ---- Invalid Error: Failed to execute query @@ -115,12 +115,12 @@ mode unskip # NULL parameters statement error -select * from postgres_query(NULL, 'SELECT 42'); +FROM postgres_query(NULL, 'SELECT 42'); ---- cannot be NULL statement error -select * from postgres_query('s1', NULL); +FROM postgres_query('s1', NULL); ---- cannot be NULL @@ -129,7 +129,7 @@ statement ok ATTACH ':memory:' AS ddb statement error -select * from postgres_query('ddb', 'SELECT * FROM cars'); +FROM postgres_query('ddb', 'SELECT * FROM cars'); ---- does not refer to a Postgres database @@ -252,5 +252,95 @@ does not exist statement ok ROLLBACK +# wrapping rules + +statement ok +CALL enable_logging('PostgresQueryLog') + +# no wrap copy + +query II +FROM postgres_query('s1', 'SELECT brand, model FROM cars', query_mode='COPY_BINARY'); +---- +ferari testarosa +aston martin db2 +bentley mulsanne +ford T + +query I +SELECT COUNT(*) +FROM duckdb_logs_parsed('PostgresQueryLog') +WHERE query = 'COPY (SELECT brand, model FROM cars) TO STDOUT (FORMAT "binary");' +---- +1 + +# no wrap binary + +query II +FROM postgres_query('s1', 'SELECT brand, model FROM cars', query_mode='PLAIN_QUERY_BINARY'); +---- +ferari testarosa +aston martin db2 +bentley mulsanne +ford T + +query I +SELECT COUNT(*) +FROM duckdb_logs_parsed('PostgresQueryLog') +WHERE query = 'SELECT brand, model FROM cars' +---- +1 + +# no wrap text + +query II +FROM postgres_query('s1', 'SELECT model, brand FROM cars', query_mode='PLAIN_QUERY_TEXT'); +---- +testarosa ferari +db2 aston martin +mulsanne bentley +T ford + +query I +SELECT COUNT(*) +FROM duckdb_logs_parsed('PostgresQueryLog') +WHERE query = 'SELECT model, brand FROM cars' +---- +1 + +# wrap color_t + +query II +FROM postgres_query('s1', 'SELECT brand, color FROM cars', query_mode='PLAIN_QUERY_BINARY'); +---- +ferari red +aston martin blue +bentley gray +ford black + +query I +SELECT COUNT(*) +FROM duckdb_logs_parsed('PostgresQueryLog') +WHERE query = 'SELECT "brand", "color"::VARCHAR FROM (SELECT brand, color FROM cars) AS __unnamed_subquery ' +---- +1 + +# wrap projection + +query I +select count(*) from postgres_query('s1', 'SELECT * FROM cars WHERE color=''red''', query_mode='PLAIN_QUERY_BINARY'); +---- +1 + +query I +SELECT COUNT(*) +FROM duckdb_logs_parsed('PostgresQueryLog') +WHERE query = 'SELECT "brand" FROM (SELECT * FROM cars WHERE color=''red'') AS __unnamed_subquery ' +---- +1 + +statement ok +CALL disable_logging() + statement ok DETACH s1