From d9b3c13e2284e1792e2f4111afc633f2e11987f4 Mon Sep 17 00:00:00 2001 From: Tishj Date: Tue, 20 Feb 2024 11:05:17 +0100 Subject: [PATCH 01/12] add detection for the relkind, create a PostgresViewInfo and a PostgresViewEntry from that --- src/include/storage/postgres_create_info.hpp | 42 +++++++++++ src/include/storage/postgres_table_entry.hpp | 22 ++++-- src/include/storage/postgres_table_set.hpp | 2 +- src/include/storage/postgres_view_entry.hpp | 69 +++++++++++++++++++ src/storage/CMakeLists.txt | 4 +- src/storage/postgres_table_set.cpp | 68 +++++++++++++----- src/storage/postgres_view_entry.cpp | 32 +++++++++ .../storage/attach_information_schema.test | 19 +++++ 8 files changed, 234 insertions(+), 24 deletions(-) create mode 100644 src/include/storage/postgres_create_info.hpp create mode 100644 src/include/storage/postgres_view_entry.hpp create mode 100644 src/storage/postgres_view_entry.cpp create mode 100644 test/sql/storage/attach_information_schema.test diff --git a/src/include/storage/postgres_create_info.hpp b/src/include/storage/postgres_create_info.hpp new file mode 100644 index 00000000..e72c367b --- /dev/null +++ b/src/include/storage/postgres_create_info.hpp @@ -0,0 +1,42 @@ +//===----------------------------------------------------------------------===// +// DuckDB +// +// storage/postgres_create_info.hpp +// +// +//===----------------------------------------------------------------------===// + +#pragma once + +#include "duckdb/catalog/catalog_entry/table_catalog_entry.hpp" +#include "duckdb/parser/parsed_data/create_table_info.hpp" +#include "postgres_utils.hpp" + +namespace duckdb { + +struct PostgresCreateInfo { +public: + PostgresCreateInfo() { + } + virtual ~PostgresCreateInfo() {} +public: + virtual CreateInfo &GetCreateInfo() = 0; + virtual const string &GetName() const = 0; + virtual void AddColumn(ColumnDefinition def, PostgresType pg_type, const string &pg_name) = 0; +public: + template + TARGET &Cast() { + D_ASSERT(dynamic_cast(this)); + return reinterpret_cast(*this); + } + + template + const TARGET &Cast() const { + D_ASSERT(dynamic_cast(this)); + return reinterpret_cast(*this); + } +public: + idx_t approx_num_pages = 0; +}; + +} // namespace duckdb diff --git a/src/include/storage/postgres_table_entry.hpp b/src/include/storage/postgres_table_entry.hpp index d96dfad8..6e46a287 100644 --- a/src/include/storage/postgres_table_entry.hpp +++ b/src/include/storage/postgres_table_entry.hpp @@ -10,11 +10,13 @@ #include "duckdb/catalog/catalog_entry/table_catalog_entry.hpp" #include "duckdb/parser/parsed_data/create_table_info.hpp" +#include "storage/postgres_create_info.hpp" #include "postgres_utils.hpp" namespace duckdb { -struct PostgresTableInfo { +struct PostgresTableInfo : public PostgresCreateInfo { +public: PostgresTableInfo() { create_info = make_uniq(); create_info->columns.SetAllowDuplicates(true); @@ -27,15 +29,27 @@ struct PostgresTableInfo { create_info = make_uniq((SchemaCatalogEntry &)schema, table); create_info->columns.SetAllowDuplicates(true); } + ~PostgresTableInfo() override {} +public: + + CreateInfo &GetCreateInfo() override { + return *create_info; + } - const string &GetTableName() const { + const string &GetName() const override { return create_info->table; } + void AddColumn(ColumnDefinition def, PostgresType pg_type, const string &pg_name) override { + postgres_types.push_back(std::move(pg_type)); + postgres_names.push_back(pg_name); + create_info->columns.AddColumn(std::move(def)); + } + +public: unique_ptr create_info; vector postgres_types; vector postgres_names; - idx_t approx_num_pages = 0; }; class PostgresTableEntry : public TableCatalogEntry { @@ -61,7 +75,7 @@ class PostgresTableEntry : public TableCatalogEntry { vector postgres_types; //! Column names as they are within Postgres //! We track these separately because of case sensitivity - Postgres allows e.g. the columns "ID" and "id" together - //! We would in this case remap them to "ID" and "id:1", while postgres_names store the original names + //! We would in this case remap them to "ID" and "id_1", while postgres_names store the original names vector postgres_names; //! The approximate number of pages a table consumes in Postgres idx_t approx_num_pages; diff --git a/src/include/storage/postgres_table_set.hpp b/src/include/storage/postgres_table_set.hpp index 590992ce..1e5a37f0 100644 --- a/src/include/storage/postgres_table_set.hpp +++ b/src/include/storage/postgres_table_set.hpp @@ -46,7 +46,7 @@ class PostgresTableSet : public PostgresInSchemaSet { void AlterTable(ClientContext &context, RemoveColumnInfo &info); static void AddColumn(optional_ptr transaction, optional_ptr schema, - PostgresResult &result, idx_t row, PostgresTableInfo &table_info, idx_t column_offset = 0); + PostgresResult &result, idx_t row, PostgresCreateInfo &pg_create_info, idx_t column_offset = 0); void CreateEntries(PostgresTransaction &transaction, PostgresResult &result, idx_t start, idx_t end); diff --git a/src/include/storage/postgres_view_entry.hpp b/src/include/storage/postgres_view_entry.hpp new file mode 100644 index 00000000..0fab788f --- /dev/null +++ b/src/include/storage/postgres_view_entry.hpp @@ -0,0 +1,69 @@ +//===----------------------------------------------------------------------===// +// DuckDB +// +// storage/postgres_view_entry.hpp +// +// +//===----------------------------------------------------------------------===// + +#pragma once + +#include "duckdb/catalog/catalog_entry/view_catalog_entry.hpp" +#include "duckdb/parser/parsed_data/create_view_info.hpp" +#include "storage/postgres_create_info.hpp" + +namespace duckdb { + +struct PostgresViewInfo : public PostgresCreateInfo { +public: + PostgresViewInfo() { + create_info = make_uniq(); + // create_info->columns.SetAllowDuplicates(true); + } + PostgresViewInfo(const string &schema, const string &view) { + create_info = make_uniq(string(), schema, view); + // create_info->columns.SetAllowDuplicates(true); + } + PostgresViewInfo(const SchemaCatalogEntry &schema, const string &view) { + create_info = make_uniq((SchemaCatalogEntry &)schema, view); + // create_info->columns.SetAllowDuplicates(true); + } +public: + + const string &GetName() const override { + return create_info->view_name; + } + + CreateInfo &GetCreateInfo() override { + return *create_info; + } + + void AddColumn(ColumnDefinition def, PostgresType pg_type, const string &pg_name) override { + postgres_types.push_back(std::move(pg_type)); + postgres_names.push_back(pg_name); + create_info->types.push_back(def.Type()); + create_info->names.push_back(def.Name()); + } + +public: + unique_ptr create_info; + vector postgres_types; + vector postgres_names; +}; + +class PostgresViewEntry : public ViewCatalogEntry { +public: + PostgresViewEntry(Catalog &catalog, SchemaCatalogEntry &schema, CreateViewInfo &info); + PostgresViewEntry(Catalog &catalog, SchemaCatalogEntry &schema, PostgresViewInfo &info); +public: + //! Postgres type annotations + vector postgres_types; + //! Column names as they are within Postgres + //! We track these separately because of case sensitivity - Postgres allows e.g. the columns "ID" and "id" together + //! We would in this case remap them to "ID" and "id_1", while postgres_names store the original names + vector postgres_names; + //! The approximate number of pages a table consumes in Postgres + idx_t approx_num_pages; +}; + +} // namespace duckdb diff --git a/src/storage/CMakeLists.txt b/src/storage/CMakeLists.txt index e6def351..38f354e8 100644 --- a/src/storage/CMakeLists.txt +++ b/src/storage/CMakeLists.txt @@ -18,7 +18,9 @@ add_library( postgres_transaction_manager.cpp postgres_type_entry.cpp postgres_type_set.cpp - postgres_update.cpp) + postgres_update.cpp + postgres_view_entry.cpp + ) set(ALL_OBJECT_FILES ${ALL_OBJECT_FILES} $ PARENT_SCOPE) diff --git a/src/storage/postgres_table_set.cpp b/src/storage/postgres_table_set.cpp index 9457c553..e34d2423 100644 --- a/src/storage/postgres_table_set.cpp +++ b/src/storage/postgres_table_set.cpp @@ -1,5 +1,7 @@ #include "storage/postgres_table_set.hpp" #include "storage/postgres_transaction.hpp" +#include "storage/postgres_table_entry.hpp" +#include "storage/postgres_view_entry.hpp" #include "duckdb/parser/parsed_data/create_table_info.hpp" #include "duckdb/parser/constraints/not_null_constraint.hpp" #include "duckdb/parser/constraints/unique_constraint.hpp" @@ -7,6 +9,7 @@ #include "duckdb/planner/parsed_data/bound_create_table_info.hpp" #include "duckdb/catalog/dependency_list.hpp" #include "duckdb/parser/parsed_data/create_table_info.hpp" +#include "duckdb/parser/parsed_data/create_view_info.hpp" #include "duckdb/parser/constraints/list.hpp" #include "storage/postgres_schema_entry.hpp" #include "duckdb/parser/parser.hpp" @@ -20,7 +23,7 @@ PostgresTableSet::PostgresTableSet(PostgresSchemaEntry &schema, unique_ptr transaction, optional_ptr schema, PostgresResult &result, idx_t row, - PostgresTableInfo &table_info, idx_t column_offset) { + PostgresCreateInfo &pg_create_info, idx_t column_offset) { PostgresTypeData type_info; idx_t column_index = column_offset; auto column_name = result.GetString(row, column_index); @@ -44,8 +47,6 @@ void PostgresTableSet::AddColumn(optional_ptr transaction, PostgresType postgres_type; auto column_type = PostgresUtils::TypeToLogicalType(transaction, schema, type_info, postgres_type); - table_info.postgres_types.push_back(std::move(postgres_type)); - table_info.postgres_names.push_back(column_name); ColumnDefinition column(std::move(column_name), std::move(column_type)); if (!default_value.empty()) { auto expressions = Parser::ParseExpressionList(default_value); @@ -54,32 +55,63 @@ void PostgresTableSet::AddColumn(optional_ptr transaction, } column.SetDefaultValue(std::move(expressions[0])); } - auto &create_info = *table_info.create_info; - create_info.columns.AddColumn(std::move(column)); + pg_create_info.AddColumn(std::move(column), std::move(postgres_type), column_name); +} + +static CatalogType TransformRelKind(const string &relkind) { + if (relkind == "v") { + return CatalogType::VIEW_ENTRY; + } + // For now we only support views and tables + return CatalogType::TABLE_ENTRY; } void PostgresTableSet::CreateEntries(PostgresTransaction &transaction, PostgresResult &result, idx_t start, idx_t end) { - vector> tables; - unique_ptr info; + vector> infos; + unique_ptr info; for (idx_t row = start; row < end; row++) { - auto table_name = result.GetString(row, 1); - auto approx_num_pages = result.GetInt64(row, 2); - if (!info || info->GetTableName() != table_name) { + auto relname = result.GetString(row, 1); + auto relkind = result.GetString(row, 2); + auto approx_num_pages = result.GetInt64(row, 3); + if (!info || info->GetName() != relname) { if (info) { - tables.push_back(std::move(info)); + infos.push_back(std::move(info)); + } + auto catalog_type = TransformRelKind(relkind); + switch (catalog_type) { + case CatalogType::TABLE_ENTRY: + info = make_uniq(schema, relname); + break; + case CatalogType::VIEW_ENTRY: + info = make_uniq(schema, relname); + break; + default: { + throw InternalException("Unexpected CatalogType in CreateEntries: %s", CatalogTypeToString(catalog_type)); + } + info->approx_num_pages = approx_num_pages; } - info = make_uniq(schema, table_name); - info->approx_num_pages = approx_num_pages; } AddColumn(&transaction, &schema, result, row, *info, 3); } if (info) { - tables.push_back(std::move(info)); + infos.push_back(std::move(info)); } - for (auto &tbl_info : tables) { - auto table_entry = make_uniq(catalog, schema, *tbl_info); - CreateEntry(std::move(table_entry)); + for (auto &pg_create_info : infos) { + auto &create_info = pg_create_info->GetCreateInfo(); + unique_ptr catalog_entry; + switch (create_info.type) { + case CatalogType::TABLE_ENTRY: + catalog_entry = make_uniq(catalog, schema, pg_create_info->Cast()); + break; + case CatalogType::VIEW_ENTRY: + catalog_entry = make_uniq(catalog, schema, pg_create_info->Cast()); + break; + default: { + throw InternalException("Unexpected CatalogType in CreateInfo: %s", CatalogTypeToString(create_info.type)); + } + } + CreateEntry(std::move(catalog_entry)); } } diff --git a/src/storage/postgres_view_entry.cpp b/src/storage/postgres_view_entry.cpp new file mode 100644 index 00000000..1e24877c --- /dev/null +++ b/src/storage/postgres_view_entry.cpp @@ -0,0 +1,32 @@ +#include "storage/postgres_catalog.hpp" +#include "storage/postgres_view_entry.hpp" +#include "storage/postgres_transaction.hpp" +#include "duckdb/storage/statistics/base_statistics.hpp" +#include "duckdb/catalog/catalog_entry/view_catalog_entry.hpp" +#include "postgres_scanner.hpp" + +namespace duckdb { + +PostgresViewEntry::PostgresViewEntry(Catalog &catalog, SchemaCatalogEntry &schema, CreateViewInfo &info) + : ViewCatalogEntry(catalog, schema, info) { + idx_t column_count = types.size(); + for (idx_t c = 0; c < column_count; c++) { + auto &type = types[c]; + auto &name = names[c]; + if (type.HasAlias()) { + type = PostgresUtils::RemoveAlias(type); + } + postgres_types.push_back(PostgresUtils::CreateEmptyPostgresType(type)); + postgres_names.push_back(name); + } + approx_num_pages = 0; +} + +PostgresViewEntry::PostgresViewEntry(Catalog &catalog, SchemaCatalogEntry &schema, PostgresViewInfo &info) + : ViewCatalogEntry(catalog, schema, *info.create_info), postgres_types(std::move(info.postgres_types)), + postgres_names(std::move(info.postgres_names)) { + D_ASSERT(postgres_types.size() == types.size()); + approx_num_pages = info.approx_num_pages; +} + +} // namespace duckdb diff --git a/test/sql/storage/attach_information_schema.test b/test/sql/storage/attach_information_schema.test new file mode 100644 index 00000000..41d9b31e --- /dev/null +++ b/test/sql/storage/attach_information_schema.test @@ -0,0 +1,19 @@ +# name: test/sql/storage/attach_describe.test +# description: Test DESCRIBE +# group: [storage] + +require postgres_scanner + +require-env POSTGRES_TEST_DATABASE_AVAILABLE + +statement ok +PRAGMA enable_verification + +statement ok +ATTACH 'dbname=postgres' AS s1 (TYPE POSTGRES) + +query IIII +select table_catalog, table_schema, table_name, table_type from information_schema.tables where table_name in ('v','s','t'); +---- +s1 public t BASE TABLE +s1 public v VIEW From 67a2c87c5312085742e014b3e8a89c5d104b6b7f Mon Sep 17 00:00:00 2001 From: Tishj Date: Tue, 20 Feb 2024 11:14:40 +0100 Subject: [PATCH 02/12] format --- src/include/storage/postgres_create_info.hpp | 6 +++- src/include/storage/postgres_table_entry.hpp | 5 +-- src/include/storage/postgres_table_set.hpp | 3 +- src/include/storage/postgres_view_entry.hpp | 3 +- src/storage/postgres_catalog_set.cpp | 4 +-- src/storage/postgres_schema_entry.cpp | 8 ++--- src/storage/postgres_schema_set.cpp | 15 ++++---- src/storage/postgres_table_set.cpp | 37 ++++++++++---------- 8 files changed, 44 insertions(+), 37 deletions(-) diff --git a/src/include/storage/postgres_create_info.hpp b/src/include/storage/postgres_create_info.hpp index e72c367b..454b3855 100644 --- a/src/include/storage/postgres_create_info.hpp +++ b/src/include/storage/postgres_create_info.hpp @@ -18,11 +18,14 @@ struct PostgresCreateInfo { public: PostgresCreateInfo() { } - virtual ~PostgresCreateInfo() {} + virtual ~PostgresCreateInfo() { + } + public: virtual CreateInfo &GetCreateInfo() = 0; virtual const string &GetName() const = 0; virtual void AddColumn(ColumnDefinition def, PostgresType pg_type, const string &pg_name) = 0; + public: template TARGET &Cast() { @@ -35,6 +38,7 @@ struct PostgresCreateInfo { D_ASSERT(dynamic_cast(this)); return reinterpret_cast(*this); } + public: idx_t approx_num_pages = 0; }; diff --git a/src/include/storage/postgres_table_entry.hpp b/src/include/storage/postgres_table_entry.hpp index 6e46a287..fc14f251 100644 --- a/src/include/storage/postgres_table_entry.hpp +++ b/src/include/storage/postgres_table_entry.hpp @@ -29,9 +29,10 @@ struct PostgresTableInfo : public PostgresCreateInfo { create_info = make_uniq((SchemaCatalogEntry &)schema, table); create_info->columns.SetAllowDuplicates(true); } - ~PostgresTableInfo() override {} -public: + ~PostgresTableInfo() override { + } +public: CreateInfo &GetCreateInfo() override { return *create_info; } diff --git a/src/include/storage/postgres_table_set.hpp b/src/include/storage/postgres_table_set.hpp index 1e5a37f0..c377ab19 100644 --- a/src/include/storage/postgres_table_set.hpp +++ b/src/include/storage/postgres_table_set.hpp @@ -46,7 +46,8 @@ class PostgresTableSet : public PostgresInSchemaSet { void AlterTable(ClientContext &context, RemoveColumnInfo &info); static void AddColumn(optional_ptr transaction, optional_ptr schema, - PostgresResult &result, idx_t row, PostgresCreateInfo &pg_create_info, idx_t column_offset = 0); + PostgresResult &result, idx_t row, PostgresCreateInfo &pg_create_info, + idx_t column_offset = 0); void CreateEntries(PostgresTransaction &transaction, PostgresResult &result, idx_t start, idx_t end); diff --git a/src/include/storage/postgres_view_entry.hpp b/src/include/storage/postgres_view_entry.hpp index 0fab788f..b67a39b1 100644 --- a/src/include/storage/postgres_view_entry.hpp +++ b/src/include/storage/postgres_view_entry.hpp @@ -28,8 +28,8 @@ struct PostgresViewInfo : public PostgresCreateInfo { create_info = make_uniq((SchemaCatalogEntry &)schema, view); // create_info->columns.SetAllowDuplicates(true); } -public: +public: const string &GetName() const override { return create_info->view_name; } @@ -55,6 +55,7 @@ class PostgresViewEntry : public ViewCatalogEntry { public: PostgresViewEntry(Catalog &catalog, SchemaCatalogEntry &schema, CreateViewInfo &info); PostgresViewEntry(Catalog &catalog, SchemaCatalogEntry &schema, PostgresViewInfo &info); + public: //! Postgres type annotations vector postgres_types; diff --git a/src/storage/postgres_catalog_set.cpp b/src/storage/postgres_catalog_set.cpp index 8ba4732d..80726f04 100644 --- a/src/storage/postgres_catalog_set.cpp +++ b/src/storage/postgres_catalog_set.cpp @@ -105,8 +105,8 @@ void PostgresCatalogSet::ClearEntries() { is_loaded = false; } -PostgresInSchemaSet::PostgresInSchemaSet(PostgresSchemaEntry &schema, bool is_loaded) : - PostgresCatalogSet(schema.ParentCatalog(), is_loaded), schema(schema) { +PostgresInSchemaSet::PostgresInSchemaSet(PostgresSchemaEntry &schema, bool is_loaded) + : PostgresCatalogSet(schema.ParentCatalog(), is_loaded), schema(schema) { } optional_ptr PostgresInSchemaSet::CreateEntry(unique_ptr entry) { diff --git a/src/storage/postgres_schema_entry.cpp b/src/storage/postgres_schema_entry.cpp index 49e89c65..3946f424 100644 --- a/src/storage/postgres_schema_entry.cpp +++ b/src/storage/postgres_schema_entry.cpp @@ -17,12 +17,12 @@ PostgresSchemaEntry::PostgresSchemaEntry(Catalog &catalog, CreateSchemaInfo &inf : SchemaCatalogEntry(catalog, info), tables(*this), indexes(*this), types(*this) { } -PostgresSchemaEntry::PostgresSchemaEntry(Catalog &catalog, CreateSchemaInfo &info, unique_ptr tables, - unique_ptr enums, +PostgresSchemaEntry::PostgresSchemaEntry(Catalog &catalog, CreateSchemaInfo &info, + unique_ptr tables, unique_ptr enums, unique_ptr composite_types, unique_ptr indexes) - : SchemaCatalogEntry(catalog, info), tables(*this, std::move(tables)), - indexes(*this, std::move(indexes)), types(*this, std::move(enums), std::move(composite_types)) { + : SchemaCatalogEntry(catalog, info), tables(*this, std::move(tables)), indexes(*this, std::move(indexes)), + types(*this, std::move(enums), std::move(composite_types)) { } bool PostgresSchemaEntry::SchemaIsInternal(const string &name) { diff --git a/src/storage/postgres_schema_set.cpp b/src/storage/postgres_schema_set.cpp index 2c3c85f9..940407b0 100644 --- a/src/storage/postgres_schema_set.cpp +++ b/src/storage/postgres_schema_set.cpp @@ -61,12 +61,11 @@ void PostgresSchemaSet::LoadEntries(ClientContext &context) { for (idx_t row = 0; row < rows; row++) { auto oid = result->GetInt64(row, 0); auto schema_name = result->GetString(row, 1); - CreateSchemaInfo info; - info.schema = schema_name; - info.internal = PostgresSchemaEntry::SchemaIsInternal(schema_name); - auto schema = - make_uniq(catalog, info, std::move(tables[row]), std::move(enums[row]), - std::move(composite_types[row]), std::move(indexes[row])); + CreateSchemaInfo info; + info.schema = schema_name; + info.internal = PostgresSchemaEntry::SchemaIsInternal(schema_name); + auto schema = make_uniq(catalog, info, std::move(tables[row]), std::move(enums[row]), + std::move(composite_types[row]), std::move(indexes[row])); CreateEntry(std::move(schema)); } } @@ -76,8 +75,8 @@ optional_ptr PostgresSchemaSet::CreateSchema(ClientContext &contex string create_sql = "CREATE SCHEMA " + KeywordHelper::WriteQuoted(info.schema, '"'); transaction.Query(create_sql); - auto info_copy = info.Copy(); - info.internal = PostgresSchemaEntry::SchemaIsInternal(info_copy->schema); + auto info_copy = info.Copy(); + info.internal = PostgresSchemaEntry::SchemaIsInternal(info_copy->schema); auto schema_entry = make_uniq(catalog, info_copy->Cast()); return CreateEntry(std::move(schema_entry)); } diff --git a/src/storage/postgres_table_set.cpp b/src/storage/postgres_table_set.cpp index e34d2423..2c062d25 100644 --- a/src/storage/postgres_table_set.cpp +++ b/src/storage/postgres_table_set.cpp @@ -80,15 +80,16 @@ void PostgresTableSet::CreateEntries(PostgresTransaction &transaction, PostgresR } auto catalog_type = TransformRelKind(relkind); switch (catalog_type) { - case CatalogType::TABLE_ENTRY: - info = make_uniq(schema, relname); - break; - case CatalogType::VIEW_ENTRY: - info = make_uniq(schema, relname); - break; - default: { - throw InternalException("Unexpected CatalogType in CreateEntries: %s", CatalogTypeToString(catalog_type)); - } + case CatalogType::TABLE_ENTRY: + info = make_uniq(schema, relname); + break; + case CatalogType::VIEW_ENTRY: + info = make_uniq(schema, relname); + break; + default: { + throw InternalException("Unexpected CatalogType in CreateEntries: %s", + CatalogTypeToString(catalog_type)); + } info->approx_num_pages = approx_num_pages; } } @@ -101,15 +102,15 @@ void PostgresTableSet::CreateEntries(PostgresTransaction &transaction, PostgresR auto &create_info = pg_create_info->GetCreateInfo(); unique_ptr catalog_entry; switch (create_info.type) { - case CatalogType::TABLE_ENTRY: - catalog_entry = make_uniq(catalog, schema, pg_create_info->Cast()); - break; - case CatalogType::VIEW_ENTRY: - catalog_entry = make_uniq(catalog, schema, pg_create_info->Cast()); - break; - default: { - throw InternalException("Unexpected CatalogType in CreateInfo: %s", CatalogTypeToString(create_info.type)); - } + case CatalogType::TABLE_ENTRY: + catalog_entry = make_uniq(catalog, schema, pg_create_info->Cast()); + break; + case CatalogType::VIEW_ENTRY: + catalog_entry = make_uniq(catalog, schema, pg_create_info->Cast()); + break; + default: { + throw InternalException("Unexpected CatalogType in CreateInfo: %s", CatalogTypeToString(create_info.type)); + } } CreateEntry(std::move(catalog_entry)); } From 28e2fd8851d5d7e768d1ed906d0aaf8ddb4937ab Mon Sep 17 00:00:00 2001 From: Tishj Date: Tue, 20 Feb 2024 11:22:41 +0100 Subject: [PATCH 03/12] add a view to the 'create-postgres-tables.sh' script, adjust test --- test/other.sql | 2 ++ test/sql/storage/attach_information_schema.test | 11 ++++++++--- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/test/other.sql b/test/other.sql index bb4be381..7ac3366a 100644 --- a/test/other.sql +++ b/test/other.sql @@ -131,6 +131,8 @@ CREATE TABLE dum(); CREATE TABLE dee(); INSERT INTO dee DEFAULT VALUES; +CREATE VIEW dee_view as select * from dee; + -- table with duplicate column names CREATE TABLE tbl_with_case_sensitive_columns ( "MyColumn" INT, diff --git a/test/sql/storage/attach_information_schema.test b/test/sql/storage/attach_information_schema.test index 41d9b31e..01bda18a 100644 --- a/test/sql/storage/attach_information_schema.test +++ b/test/sql/storage/attach_information_schema.test @@ -13,7 +13,12 @@ statement ok ATTACH 'dbname=postgres' AS s1 (TYPE POSTGRES) query IIII -select table_catalog, table_schema, table_name, table_type from information_schema.tables where table_name in ('v','s','t'); +select + table_catalog, + table_schema, + table_name, + table_type +from information_schema.tables where table_name in ('v','s','t') and table_name LIKE 'dee%'; ---- -s1 public t BASE TABLE -s1 public v VIEW +s1 public dee BASE TABLE +s1 public dee_view VIEW From 84511fba7df2c6633b9a6101cb1ee6a73219f5af Mon Sep 17 00:00:00 2001 From: Tishj Date: Tue, 20 Feb 2024 11:32:08 +0100 Subject: [PATCH 04/12] override the destructor --- src/include/storage/postgres_view_entry.hpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/include/storage/postgres_view_entry.hpp b/src/include/storage/postgres_view_entry.hpp index b67a39b1..dd03b4ff 100644 --- a/src/include/storage/postgres_view_entry.hpp +++ b/src/include/storage/postgres_view_entry.hpp @@ -28,8 +28,9 @@ struct PostgresViewInfo : public PostgresCreateInfo { create_info = make_uniq((SchemaCatalogEntry &)schema, view); // create_info->columns.SetAllowDuplicates(true); } - + ~PostgresViewInfo() override {} public: + const string &GetName() const override { return create_info->view_name; } @@ -55,7 +56,6 @@ class PostgresViewEntry : public ViewCatalogEntry { public: PostgresViewEntry(Catalog &catalog, SchemaCatalogEntry &schema, CreateViewInfo &info); PostgresViewEntry(Catalog &catalog, SchemaCatalogEntry &schema, PostgresViewInfo &info); - public: //! Postgres type annotations vector postgres_types; From 8894e28d1e4ba19715f131a82af0588599d276a8 Mon Sep 17 00:00:00 2001 From: Tishj Date: Thu, 22 Feb 2024 16:34:00 +0100 Subject: [PATCH 05/12] updated test --- duckdb | 2 +- src/include/postgres_binary_reader.hpp | 9 ++--- src/include/storage/postgres_schema_entry.hpp | 2 +- src/include/storage/postgres_view_entry.hpp | 6 ++-- src/postgres_extension.cpp | 12 +++---- src/postgres_scanner.cpp | 5 +-- src/storage/postgres_index.cpp | 34 ++++++++++--------- src/storage/postgres_optimizer.cpp | 9 ++--- test/other.sql | 4 +-- .../storage/attach_information_schema.test | 8 ++--- 10 files changed, 48 insertions(+), 43 deletions(-) diff --git a/duckdb b/duckdb index 0e784765..336c9be1 160000 --- a/duckdb +++ b/duckdb @@ -1 +1 @@ -Subproject commit 0e784765f6f87bd1ce9034afcce1e7f89fcd8777 +Subproject commit 336c9be1d2a7e419990d1d68634bc86f212d6b2b diff --git a/src/include/postgres_binary_reader.hpp b/src/include/postgres_binary_reader.hpp index 4737ed92..de6670db 100644 --- a/src/include/postgres_binary_reader.hpp +++ b/src/include/postgres_binary_reader.hpp @@ -263,9 +263,10 @@ struct PostgresBinaryReader { return (config.is_negative ? -base_res : base_res); } - void ReadGeometry(const LogicalType &type, const PostgresType &postgres_type, Vector &out_vec, idx_t output_offset) { + void ReadGeometry(const LogicalType &type, const PostgresType &postgres_type, Vector &out_vec, + idx_t output_offset) { idx_t element_count = 0; - switch(postgres_type.info) { + switch (postgres_type.info) { case PostgresTypeAnnotation::GEOM_LINE: case PostgresTypeAnnotation::GEOM_CIRCLE: element_count = 3; @@ -294,7 +295,7 @@ struct PostgresBinaryReader { list_entries[output_offset].length = element_count; auto &child_vector = ListVector::GetEntry(out_vec); auto child_data = FlatVector::GetData(child_vector); - for(idx_t i = 0; i < element_count; i++) { + for (idx_t i = 0; i < element_count; i++) { child_data[child_offset + i] = ReadDouble(); } ListVector::SetListSize(out_vec, child_offset + element_count); @@ -488,7 +489,7 @@ struct PostgresBinaryReader { list_entry.length = 0; break; } - switch(postgres_type.info) { + switch (postgres_type.info) { case PostgresTypeAnnotation::GEOM_LINE: case PostgresTypeAnnotation::GEOM_LINE_SEGMENT: case PostgresTypeAnnotation::GEOM_BOX: diff --git a/src/include/storage/postgres_schema_entry.hpp b/src/include/storage/postgres_schema_entry.hpp index ab9d20b5..e9963ef7 100644 --- a/src/include/storage/postgres_schema_entry.hpp +++ b/src/include/storage/postgres_schema_entry.hpp @@ -44,7 +44,7 @@ class PostgresSchemaEntry : public SchemaCatalogEntry { void DropEntry(ClientContext &context, DropInfo &info) override; optional_ptr GetEntry(CatalogTransaction transaction, CatalogType type, const string &name) override; - static bool SchemaIsInternal(const string &name); + static bool SchemaIsInternal(const string &name); private: void AlterTable(PostgresTransaction &transaction, RenameTableInfo &info); diff --git a/src/include/storage/postgres_view_entry.hpp b/src/include/storage/postgres_view_entry.hpp index dd03b4ff..54ec638e 100644 --- a/src/include/storage/postgres_view_entry.hpp +++ b/src/include/storage/postgres_view_entry.hpp @@ -28,9 +28,10 @@ struct PostgresViewInfo : public PostgresCreateInfo { create_info = make_uniq((SchemaCatalogEntry &)schema, view); // create_info->columns.SetAllowDuplicates(true); } - ~PostgresViewInfo() override {} -public: + ~PostgresViewInfo() override { + } +public: const string &GetName() const override { return create_info->view_name; } @@ -56,6 +57,7 @@ class PostgresViewEntry : public ViewCatalogEntry { public: PostgresViewEntry(Catalog &catalog, SchemaCatalogEntry &schema, CreateViewInfo &info); PostgresViewEntry(Catalog &catalog, SchemaCatalogEntry &schema, PostgresViewInfo &info); + public: //! Postgres type annotations vector postgres_types; diff --git a/src/postgres_extension.cpp b/src/postgres_extension.cpp index 81bd46ed..29e282b2 100644 --- a/src/postgres_extension.cpp +++ b/src/postgres_extension.cpp @@ -62,19 +62,17 @@ static void LoadInternal(DatabaseInstance &db) { config.AddExtensionOption("pg_connection_limit", "The maximum amount of concurrent Postgres connections", LogicalType::UBIGINT, Value::UBIGINT(PostgresConnectionPool::DEFAULT_MAX_CONNECTIONS), SetPostgresConnectionLimit); - config.AddExtensionOption("pg_array_as_varchar", - "Read Postgres arrays as varchar - enables reading mixed dimensional arrays", - LogicalType::BOOLEAN, Value::BOOLEAN(false), PostgresClearCacheFunction::ClearCacheOnSetting); - config.AddExtensionOption("pg_connection_cache", - "Whether or not to use the connection cache", LogicalType::BOOLEAN, - Value::BOOLEAN(true), PostgresConnectionPool::PostgresSetConnectionCache); + config.AddExtensionOption( + "pg_array_as_varchar", "Read Postgres arrays as varchar - enables reading mixed dimensional arrays", + LogicalType::BOOLEAN, Value::BOOLEAN(false), PostgresClearCacheFunction::ClearCacheOnSetting); + config.AddExtensionOption("pg_connection_cache", "Whether or not to use the connection cache", LogicalType::BOOLEAN, + Value::BOOLEAN(true), PostgresConnectionPool::PostgresSetConnectionCache); config.AddExtensionOption("pg_experimental_filter_pushdown", "Whether or not to use filter pushdown (currently experimental)", LogicalType::BOOLEAN, Value::BOOLEAN(false)); config.AddExtensionOption("pg_debug_show_queries", "DEBUG SETTING: print all queries sent to Postgres to stdout", LogicalType::BOOLEAN, Value::BOOLEAN(false), SetPostgresDebugQueryPrint); - OptimizerExtension postgres_optimizer; postgres_optimizer.optimize_function = PostgresOptimizer::Optimize; config.optimizer_extensions.push_back(std::move(postgres_optimizer)); diff --git a/src/postgres_scanner.cpp b/src/postgres_scanner.cpp index 9fca0967..07699d56 100644 --- a/src/postgres_scanner.cpp +++ b/src/postgres_scanner.cpp @@ -481,12 +481,13 @@ unique_ptr PostgresScanCardinality(ClientContext &context, const } double PostgresScanProgress(ClientContext &context, const FunctionData *bind_data_p, - const GlobalTableFunctionState *global_state) { + const GlobalTableFunctionState *global_state) { auto &bind_data = bind_data_p->Cast(); auto &gstate = global_state->Cast(); lock_guard parallel_lock(gstate.lock); - double progress = 100 * double(gstate.page_idx) / double(bind_data.pages_approx);; + double progress = 100 * double(gstate.page_idx) / double(bind_data.pages_approx); + ; return MinValue(100, progress); } diff --git a/src/storage/postgres_index.cpp b/src/storage/postgres_index.cpp index 4ac2860d..b5e454b4 100644 --- a/src/storage/postgres_index.cpp +++ b/src/storage/postgres_index.cpp @@ -18,23 +18,25 @@ SourceResultType PostgresCreateIndex::GetData(ExecutionContext &context, DataChu OperatorSourceInput &input) const { auto &catalog = table.catalog; auto &schema = table.schema; - auto existing = schema.GetEntry(catalog.GetCatalogTransaction(context.client), CatalogType::INDEX_ENTRY, info->index_name); + auto existing = + schema.GetEntry(catalog.GetCatalogTransaction(context.client), CatalogType::INDEX_ENTRY, info->index_name); if (existing) { - switch(info->on_conflict) { - case OnCreateConflict::IGNORE_ON_CONFLICT: - return SourceResultType::FINISHED; - case OnCreateConflict::ERROR_ON_CONFLICT: - throw BinderException("Index with name \"%s\" already exists in schema \"%s\"", info->index_name, table.schema.name); - case OnCreateConflict::REPLACE_ON_CONFLICT: { - DropInfo drop_info; - drop_info.type = CatalogType::INDEX_ENTRY; - drop_info.schema = info->schema; - drop_info.name = info->index_name; - schema.DropEntry(context.client, drop_info); - break; - } - default: - throw InternalException("Unsupported on create conflict"); + switch (info->on_conflict) { + case OnCreateConflict::IGNORE_ON_CONFLICT: + return SourceResultType::FINISHED; + case OnCreateConflict::ERROR_ON_CONFLICT: + throw BinderException("Index with name \"%s\" already exists in schema \"%s\"", info->index_name, + table.schema.name); + case OnCreateConflict::REPLACE_ON_CONFLICT: { + DropInfo drop_info; + drop_info.type = CatalogType::INDEX_ENTRY; + drop_info.schema = info->schema; + drop_info.name = info->index_name; + schema.DropEntry(context.client, drop_info); + break; + } + default: + throw InternalException("Unsupported on create conflict"); } } schema.CreateIndex(context.client, *info, table); diff --git a/src/storage/postgres_optimizer.cpp b/src/storage/postgres_optimizer.cpp index 9f73ff3b..83943e9d 100644 --- a/src/storage/postgres_optimizer.cpp +++ b/src/storage/postgres_optimizer.cpp @@ -29,12 +29,13 @@ void GatherPostgresScans(LogicalOperator &op, PostgresOperators &result) { result.scans[*catalog].push_back(get); } // recurse into children - for(auto &child : op.children) { + for (auto &child : op.children) { GatherPostgresScans(*child, result); } } -void PostgresOptimizer::Optimize(ClientContext &context, OptimizerExtensionInfo *info, unique_ptr &plan) { +void PostgresOptimizer::Optimize(ClientContext &context, OptimizerExtensionInfo *info, + unique_ptr &plan) { // look at the query plan and check if we can enable streaming query scans PostgresOperators operators; GatherPostgresScans(*plan, operators); @@ -42,10 +43,10 @@ void PostgresOptimizer::Optimize(ClientContext &context, OptimizerExtensionInfo // no scans return; } - for(auto &entry : operators.scans) { + for (auto &entry : operators.scans) { auto &catalog = entry.first; auto multiple_scans = entry.second.size() > 1; - for(auto &scan : entry.second) { + for (auto &scan : entry.second) { auto &bind_data = scan.get().bind_data->Cast(); // if there is a single scan in the plan we can always stream using the main thread // if there is more than one scan we either (1) need to materialize, or (2) cannot use the main thread diff --git a/test/other.sql b/test/other.sql index 7ac3366a..c806b7c3 100644 --- a/test/other.sql +++ b/test/other.sql @@ -131,8 +131,6 @@ CREATE TABLE dum(); CREATE TABLE dee(); INSERT INTO dee DEFAULT VALUES; -CREATE VIEW dee_view as select * from dee; - -- table with duplicate column names CREATE TABLE tbl_with_case_sensitive_columns ( "MyColumn" INT, @@ -158,6 +156,8 @@ INSERT INTO chars VALUES ('hello'), ('world'), ('maxlength1'), ('hello '), ( CREATE TABLE chars_array(c CHAR(10)[]); INSERT INTO chars_array VALUES (ARRAY['hello', 'world', 'maxlength1', 'hello ', ' ', NULL]); +CREATE VIEW chars_view as select * from chars; + -- varchar with length limit CREATE TABLE varchars_fixed_len(c VARCHAR(10)); INSERT INTO varchars_fixed_len VALUES ('hello'), ('world'), ('maxlength1'), ('hello '), (' '), (NULL); diff --git a/test/sql/storage/attach_information_schema.test b/test/sql/storage/attach_information_schema.test index 01bda18a..69dae5f6 100644 --- a/test/sql/storage/attach_information_schema.test +++ b/test/sql/storage/attach_information_schema.test @@ -10,7 +10,7 @@ statement ok PRAGMA enable_verification statement ok -ATTACH 'dbname=postgres' AS s1 (TYPE POSTGRES) +ATTACH 'postgres:dbname=postgresscanner' AS s1 query IIII select @@ -18,7 +18,7 @@ select table_schema, table_name, table_type -from information_schema.tables where table_name in ('v','s','t') and table_name LIKE 'dee%'; +from information_schema.tables where table_name in ('chars_view','chars'); ---- -s1 public dee BASE TABLE -s1 public dee_view VIEW +s1 public chars BASE TABLE +s1 public chars_view VIEW From 09269a6d44fc4defc8229e4da554c862c07ecf5d Mon Sep 17 00:00:00 2001 From: Tishj Date: Thu, 22 Feb 2024 18:08:58 +0100 Subject: [PATCH 06/12] initial tests for views --- create-postgres-tables.sh | 10 +- src/include/storage/postgres_create_info.hpp | 3 + src/include/storage/postgres_table_entry.hpp | 10 +- src/include/storage/postgres_table_set.hpp | 8 +- src/include/storage/postgres_view_entry.hpp | 9 +- src/postgres_scanner.cpp | 5 +- src/storage/postgres_index_set.cpp | 1 + src/storage/postgres_table_entry.cpp | 14 +++ src/storage/postgres_table_set.cpp | 119 ++++++++++++++----- src/storage/postgres_view_entry.cpp | 3 + test/sql/storage/view/test_view.test | 76 ++++++++++++ 11 files changed, 216 insertions(+), 42 deletions(-) create mode 100644 test/sql/storage/view/test_view.test diff --git a/create-postgres-tables.sh b/create-postgres-tables.sh index 408f292f..84e72005 100755 --- a/create-postgres-tables.sh +++ b/create-postgres-tables.sh @@ -1,4 +1,12 @@ #!/bin/bash + +# Set default value for the build type +BUILD_TYPE="release" +# If an argument is provided, use that as the build type instead +if [ $# -eq 1 ]; then + BUILD_TYPE=$1 +fi + echo " CREATE SCHEMA tpch; CREATE SCHEMA tpcds; @@ -6,7 +14,7 @@ CALL dbgen(sf=0.01, schema='tpch'); CALL dsdgen(sf=0.01, schema='tpcds'); EXPORT DATABASE '/tmp/postgresscannertmp'; " | \ -./build/release/duckdb +./build/$BUILD_TYPE/duckdb dropdb --if-exists postgresscanner createdb postgresscanner diff --git a/src/include/storage/postgres_create_info.hpp b/src/include/storage/postgres_create_info.hpp index 454b3855..b67d8ccf 100644 --- a/src/include/storage/postgres_create_info.hpp +++ b/src/include/storage/postgres_create_info.hpp @@ -25,6 +25,7 @@ struct PostgresCreateInfo { virtual CreateInfo &GetCreateInfo() = 0; virtual const string &GetName() const = 0; virtual void AddColumn(ColumnDefinition def, PostgresType pg_type, const string &pg_name) = 0; + virtual void GetColumnNamesAndTypes(vector &names, vector &types) = 0; public: template @@ -41,6 +42,8 @@ struct PostgresCreateInfo { public: idx_t approx_num_pages = 0; + vector postgres_types; + vector postgres_names; }; } // namespace duckdb diff --git a/src/include/storage/postgres_table_entry.hpp b/src/include/storage/postgres_table_entry.hpp index fc14f251..622967bb 100644 --- a/src/include/storage/postgres_table_entry.hpp +++ b/src/include/storage/postgres_table_entry.hpp @@ -43,20 +43,26 @@ struct PostgresTableInfo : public PostgresCreateInfo { void AddColumn(ColumnDefinition def, PostgresType pg_type, const string &pg_name) override { postgres_types.push_back(std::move(pg_type)); + D_ASSERT(!pg_name.empty()); postgres_names.push_back(pg_name); create_info->columns.AddColumn(std::move(def)); } + void GetColumnNamesAndTypes(vector &names, vector &types) override { + for (auto &col : create_info->columns.Logical()) { + names.push_back(col.GetName()); + types.push_back(col.GetType()); + } + } public: unique_ptr create_info; - vector postgres_types; - vector postgres_names; }; class PostgresTableEntry : public TableCatalogEntry { public: PostgresTableEntry(Catalog &catalog, SchemaCatalogEntry &schema, CreateTableInfo &info); PostgresTableEntry(Catalog &catalog, SchemaCatalogEntry &schema, PostgresTableInfo &info); + ~PostgresTableEntry() override; public: unique_ptr GetStatistics(ClientContext &context, column_t column_id) override; diff --git a/src/include/storage/postgres_table_set.hpp b/src/include/storage/postgres_table_set.hpp index c377ab19..ca93a385 100644 --- a/src/include/storage/postgres_table_set.hpp +++ b/src/include/storage/postgres_table_set.hpp @@ -24,10 +24,10 @@ class PostgresTableSet : public PostgresInSchemaSet { public: optional_ptr CreateTable(ClientContext &context, BoundCreateTableInfo &info); - static unique_ptr GetTableInfo(PostgresTransaction &transaction, PostgresSchemaEntry &schema, - const string &table_name); - static unique_ptr GetTableInfo(PostgresConnection &connection, const string &schema_name, - const string &table_name); + static unique_ptr GetTableInfo(PostgresTransaction &transaction, PostgresSchemaEntry &schema, + const string &table_name); + static unique_ptr GetTableInfo(PostgresConnection &connection, const string &schema_name, + const string &table_name); optional_ptr ReloadEntry(ClientContext &context, const string &table_name) override; void AlterTable(ClientContext &context, AlterTableInfo &info); diff --git a/src/include/storage/postgres_view_entry.hpp b/src/include/storage/postgres_view_entry.hpp index 54ec638e..f6b48bc3 100644 --- a/src/include/storage/postgres_view_entry.hpp +++ b/src/include/storage/postgres_view_entry.hpp @@ -42,21 +42,26 @@ struct PostgresViewInfo : public PostgresCreateInfo { void AddColumn(ColumnDefinition def, PostgresType pg_type, const string &pg_name) override { postgres_types.push_back(std::move(pg_type)); + D_ASSERT(!pg_name.empty()); postgres_names.push_back(pg_name); create_info->types.push_back(def.Type()); create_info->names.push_back(def.Name()); } + void GetColumnNamesAndTypes(vector &names, vector &types) override { + names = create_info->names; + types = create_info->types; + } + public: unique_ptr create_info; - vector postgres_types; - vector postgres_names; }; class PostgresViewEntry : public ViewCatalogEntry { public: PostgresViewEntry(Catalog &catalog, SchemaCatalogEntry &schema, CreateViewInfo &info); PostgresViewEntry(Catalog &catalog, SchemaCatalogEntry &schema, PostgresViewInfo &info); + ~PostgresViewEntry() override; public: //! Postgres type annotations diff --git a/src/postgres_scanner.cpp b/src/postgres_scanner.cpp index 07699d56..a9733d20 100644 --- a/src/postgres_scanner.cpp +++ b/src/postgres_scanner.cpp @@ -149,10 +149,7 @@ static unique_ptr PostgresBind(ClientContext &context, TableFuncti auto info = PostgresTableSet::GetTableInfo(con, bind_data->schema_name, bind_data->table_name); bind_data->postgres_types = info->postgres_types; - for (auto &col : info->create_info->columns.Logical()) { - names.push_back(col.GetName()); - return_types.push_back(col.GetType()); - } + info->GetColumnNamesAndTypes(names, return_types); bind_data->names = info->postgres_names; bind_data->types = return_types; bind_data->can_use_main_thread = true; diff --git a/src/storage/postgres_index_set.cpp b/src/storage/postgres_index_set.cpp index a886f5a1..d869c5a7 100644 --- a/src/storage/postgres_index_set.cpp +++ b/src/storage/postgres_index_set.cpp @@ -27,6 +27,7 @@ void PostgresIndexSet::LoadEntries(ClientContext &context) { auto &result = index_result->GetResult(); for (idx_t row = index_result->start; row < index_result->end; row++) { auto table_name = result.GetString(row, 1); + D_ASSERT(!table_name.empty()); auto index_name = result.GetString(row, 2); CreateIndexInfo info; info.schema = schema.name; diff --git a/src/storage/postgres_table_entry.cpp b/src/storage/postgres_table_entry.cpp index d791678b..0ea7e2e3 100644 --- a/src/storage/postgres_table_entry.cpp +++ b/src/storage/postgres_table_entry.cpp @@ -15,6 +15,7 @@ PostgresTableEntry::PostgresTableEntry(Catalog &catalog, SchemaCatalogEntry &sch col.TypeMutable() = PostgresUtils::RemoveAlias(col.GetType()); } postgres_types.push_back(PostgresUtils::CreateEmptyPostgresType(col.GetType())); + D_ASSERT(!col.GetName().empty()); postgres_names.push_back(col.GetName()); } approx_num_pages = 0; @@ -23,6 +24,11 @@ PostgresTableEntry::PostgresTableEntry(Catalog &catalog, SchemaCatalogEntry &sch PostgresTableEntry::PostgresTableEntry(Catalog &catalog, SchemaCatalogEntry &schema, PostgresTableInfo &info) : TableCatalogEntry(catalog, schema, *info.create_info), postgres_types(std::move(info.postgres_types)), postgres_names(std::move(info.postgres_names)) { +#ifdef DEBUG + for (auto &result_name : postgres_names) { + D_ASSERT(!result_name.empty()); + } +#endif D_ASSERT(postgres_types.size() == columns.LogicalColumnCount()); approx_num_pages = info.approx_num_pages; } @@ -48,6 +54,11 @@ TableFunction PostgresTableEntry::GetScanFunction(ClientContext &context, unique result->types.push_back(col.GetType()); } result->names = postgres_names; +#ifdef DEBUG + for (auto &result_name : result->names) { + D_ASSERT(!result_name.empty()); + } +#endif result->postgres_types = postgres_types; result->read_only = transaction.IsReadOnly(); PostgresScanFunction::PrepareBind(pg_catalog.GetPostgresVersion(), context, *result, approx_num_pages); @@ -120,4 +131,7 @@ PostgresCopyFormat PostgresTableEntry::GetCopyFormat(ClientContext &context) { return PostgresCopyFormat::BINARY; } +PostgresTableEntry::~PostgresTableEntry() { +} + } // namespace duckdb diff --git a/src/storage/postgres_table_set.cpp b/src/storage/postgres_table_set.cpp index 2c062d25..3aa428f2 100644 --- a/src/storage/postgres_table_set.cpp +++ b/src/storage/postgres_table_set.cpp @@ -2,16 +2,15 @@ #include "storage/postgres_transaction.hpp" #include "storage/postgres_table_entry.hpp" #include "storage/postgres_view_entry.hpp" +#include "storage/postgres_schema_entry.hpp" #include "duckdb/parser/parsed_data/create_table_info.hpp" #include "duckdb/parser/constraints/not_null_constraint.hpp" #include "duckdb/parser/constraints/unique_constraint.hpp" #include "duckdb/parser/expression/constant_expression.hpp" #include "duckdb/planner/parsed_data/bound_create_table_info.hpp" #include "duckdb/catalog/dependency_list.hpp" -#include "duckdb/parser/parsed_data/create_table_info.hpp" #include "duckdb/parser/parsed_data/create_view_info.hpp" #include "duckdb/parser/constraints/list.hpp" -#include "storage/postgres_schema_entry.hpp" #include "duckdb/parser/parser.hpp" #include "postgres_conversion.hpp" @@ -40,6 +39,7 @@ void PostgresTableSet::AddColumn(optional_ptr transaction, PostgresTypeData type_info; idx_t column_index = column_offset; auto column_name = result.GetString(row, column_index); + D_ASSERT(!column_name.empty()); type_info.type_name = result.GetString(row, column_index + 1); type_info.type_modifier = result.GetInt64(row, column_index + 2); type_info.array_dimensions = result.GetInt64(row, column_index + 3); @@ -47,7 +47,7 @@ void PostgresTableSet::AddColumn(optional_ptr transaction, PostgresType postgres_type; auto column_type = PostgresUtils::TypeToLogicalType(transaction, schema, type_info, postgres_type); - ColumnDefinition column(std::move(column_name), std::move(column_type)); + ColumnDefinition column(column_name, std::move(column_type)); if (!default_value.empty()) { auto expressions = Parser::ParseExpressionList(default_value); if (expressions.empty()) { @@ -55,15 +55,29 @@ void PostgresTableSet::AddColumn(optional_ptr transaction, } column.SetDefaultValue(std::move(expressions[0])); } - pg_create_info.AddColumn(std::move(column), std::move(postgres_type), column_name); + pg_create_info.AddColumn(std::move(column), std::move(postgres_type), std::move(column_name)); } static CatalogType TransformRelKind(const string &relkind) { if (relkind == "v") { return CatalogType::VIEW_ENTRY; } - // For now we only support views and tables - return CatalogType::TABLE_ENTRY; + if (relkind == "r") { + return CatalogType::TABLE_ENTRY; + } + if (relkind == "m") { + // TODO: support materialized views + return CatalogType::TABLE_ENTRY; + } + if (relkind == "f") { + // TODO: support foreign tables + return CatalogType::TABLE_ENTRY; + } + if (relkind == "p") { + // TODO: support partitioned tables + return CatalogType::TABLE_ENTRY; + } + throw InternalException("Unexpected relkind in TransformRelkind: %s", relkind); } void PostgresTableSet::CreateEntries(PostgresTransaction &transaction, PostgresResult &result, idx_t start, idx_t end) { @@ -71,9 +85,11 @@ void PostgresTableSet::CreateEntries(PostgresTransaction &transaction, PostgresR unique_ptr info; for (idx_t row = start; row < end; row++) { - auto relname = result.GetString(row, 1); - auto relkind = result.GetString(row, 2); - auto approx_num_pages = result.GetInt64(row, 3); + const idx_t COLUMN_OFFSET = 4; + auto relname = result.GetString(row, COLUMN_OFFSET - 3); + D_ASSERT(!relname.empty()); + auto relkind = result.GetString(row, COLUMN_OFFSET - 2); + auto approx_num_pages = result.GetInt64(row, COLUMN_OFFSET - 1); if (!info || info->GetName() != relname) { if (info) { infos.push_back(std::move(info)); @@ -93,7 +109,7 @@ void PostgresTableSet::CreateEntries(PostgresTransaction &transaction, PostgresR info->approx_num_pages = approx_num_pages; } } - AddColumn(&transaction, &schema, result, row, *info, 3); + AddColumn(&transaction, &schema, result, row, *info, COLUMN_OFFSET); } if (info) { infos.push_back(std::move(info)); @@ -143,7 +159,7 @@ void PostgresTableSet::LoadEntries(ClientContext &context) { string GetTableInfoQuery(const string &schema_name, const string &table_name) { return StringUtil::Replace(StringUtil::Replace(R"( -SELECT relpages, attname, +SELECT relpages, relkind, attname, pg_type.typname type_name, atttypmod type_modifier, pg_attribute.attndims ndim FROM pg_class JOIN pg_namespace ON relnamespace = pg_namespace.oid @@ -156,48 +172,93 @@ ORDER BY relname, attnum; "${TABLE_NAME}", KeywordHelper::WriteQuoted(table_name)); } -unique_ptr PostgresTableSet::GetTableInfo(PostgresTransaction &transaction, - PostgresSchemaEntry &schema, const string &table_name) { +unique_ptr PostgresTableSet::GetTableInfo(PostgresTransaction &transaction, + PostgresSchemaEntry &schema, const string &table_name) { auto query = GetTableInfoQuery(schema.name, table_name); auto result = transaction.Query(query); auto rows = result->Count(); if (rows == 0) { return nullptr; } - auto table_info = make_uniq(schema, table_name); + const idx_t COLUMN_OFFSET = 2; + auto relkind = result->GetString(0, 2); + auto catalog_type = TransformRelKind(relkind); + unique_ptr info; + switch (catalog_type) { + case CatalogType::TABLE_ENTRY: + info = make_uniq(schema, table_name); + break; + case CatalogType::VIEW_ENTRY: + info = make_uniq(schema, table_name); + break; + default: { + throw InternalException("Unexpected CatalogType in GetTableInfo: %s", CatalogTypeToString(catalog_type)); + } + } + for (idx_t row = 0; row < rows; row++) { - AddColumn(&transaction, &schema, *result, row, *table_info, 1); + AddColumn(&transaction, &schema, *result, row, *info, COLUMN_OFFSET); } - table_info->approx_num_pages = result->GetInt64(0, 0); - return table_info; + info->approx_num_pages = result->GetInt64(0, 0); + return info; } -unique_ptr PostgresTableSet::GetTableInfo(PostgresConnection &connection, const string &schema_name, - const string &table_name) { +unique_ptr PostgresTableSet::GetTableInfo(PostgresConnection &connection, const string &schema_name, + const string &table_name) { auto query = GetTableInfoQuery(schema_name, table_name); auto result = connection.Query(query); auto rows = result->Count(); if (rows == 0) { throw InvalidInputException("Table %s does not contain any columns.", table_name); } - auto table_info = make_uniq(schema_name, table_name); + auto relkind = result->GetString(0, 2); + auto catalog_type = TransformRelKind(relkind); + unique_ptr info; + switch (catalog_type) { + case CatalogType::TABLE_ENTRY: + info = make_uniq(schema_name, table_name); + break; + case CatalogType::VIEW_ENTRY: + info = make_uniq(schema_name, table_name); + break; + default: { + throw InternalException("Unexpected CatalogType in GetTableInfo: %s", CatalogTypeToString(catalog_type)); + } + } for (idx_t row = 0; row < rows; row++) { - AddColumn(nullptr, nullptr, *result, row, *table_info, 1); + AddColumn(nullptr, nullptr, *result, row, *info, 1); } - table_info->approx_num_pages = result->GetInt64(0, 0); - return table_info; + info->approx_num_pages = result->GetInt64(0, 0); + return info; } optional_ptr PostgresTableSet::ReloadEntry(ClientContext &context, const string &table_name) { auto &transaction = PostgresTransaction::Get(context, catalog); - auto table_info = GetTableInfo(transaction, schema, table_name); - if (!table_info) { + auto pg_info = GetTableInfo(transaction, schema, table_name); + if (!pg_info) { return nullptr; } - auto table_entry = make_uniq(catalog, schema, *table_info); - auto table_ptr = table_entry.get(); - CreateEntry(std::move(table_entry)); - return table_ptr; + + unique_ptr entry; + auto &create_info = pg_info->GetCreateInfo(); + switch (create_info.type) { + case CatalogType::TABLE_ENTRY: { + auto &table_info = pg_info->Cast(); + entry = make_uniq(catalog, schema, table_info); + break; + } + case CatalogType::VIEW_ENTRY: { + auto &view_info = pg_info->Cast(); + entry = make_uniq(catalog, schema, view_info); + break; + } + default: { + throw InternalException("Unexpected CatalogType in ReloadEntry: %s", CatalogTypeToString(create_info.type)); + } + } + auto entry_ptr = entry.get(); + CreateEntry(std::move(entry)); + return entry_ptr; } // FIXME - this is almost entirely copied from TableCatalogEntry::ColumnsToSQL - should be unified diff --git a/src/storage/postgres_view_entry.cpp b/src/storage/postgres_view_entry.cpp index 1e24877c..cfd6659f 100644 --- a/src/storage/postgres_view_entry.cpp +++ b/src/storage/postgres_view_entry.cpp @@ -29,4 +29,7 @@ PostgresViewEntry::PostgresViewEntry(Catalog &catalog, SchemaCatalogEntry &schem approx_num_pages = info.approx_num_pages; } +PostgresViewEntry::~PostgresViewEntry() { +} + } // namespace duckdb diff --git a/test/sql/storage/view/test_view.test b/test/sql/storage/view/test_view.test new file mode 100644 index 00000000..84b4709f --- /dev/null +++ b/test/sql/storage/view/test_view.test @@ -0,0 +1,76 @@ +# name: test/sql/catalog/view/test_view.test +# description: Test view creation +# group: [view] + +require postgres_scanner + +require-env POSTGRES_TEST_DATABASE_AVAILABLE + +statement ok +PRAGMA enable_verification + +statement ok +ATTACH 'postgres:dbname=postgresscanner' AS s1 + +statement ok +drop view if exists s1.v1; + +statement ok +CREATE VIEW s1.v1 AS SELECT 42 as j + +statement error +CREATE VIEW s1.v1 AS SELECT 'whatever' +---- +Failed to execute query + +query I +SELECT j FROM s1.v1 WHERE j > 41 +---- +42 + +# name alias in view +query I +SELECT x FROM s1.v1 t1(x) WHERE x > 41 +---- +42 + +statement ok +DROP VIEW s1.v1 + +statement error +SELECT j FROM s1.v1 WHERE j > 41 +---- + +statement ok +CREATE VIEW s1.v1 AS SELECT 'whatever' + +query T +SELECT * FROM s1.v1 +---- +whatever + +statement ok +CREATE OR REPLACE VIEW s1.v1 AS SELECT 42 + +query I +SELECT * FROM s1.v1 +---- +42 + +statement error +INSERT INTO s1.v1 VALUES (1) +---- + +statement ok +DROP VIEW s1.v1 + +statement error +DROP VIEW s1.v1 +---- + +statement ok +DROP VIEW IF EXISTS s1.v1 + +statement error +CREATE VIEW s1.v1 AS SELECT * FROM dontexist +---- From 8faa76f8df606624ce77e79e139eee88caf1c732 Mon Sep 17 00:00:00 2001 From: Tishj Date: Thu, 22 Feb 2024 18:42:58 +0100 Subject: [PATCH 07/12] more view tests --- src/storage/postgres_table_set.cpp | 4 +- test/sql/storage/view/test_view_sql.test | 64 ++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 2 deletions(-) create mode 100644 test/sql/storage/view/test_view_sql.test diff --git a/src/storage/postgres_table_set.cpp b/src/storage/postgres_table_set.cpp index 3aa428f2..b14f6629 100644 --- a/src/storage/postgres_table_set.cpp +++ b/src/storage/postgres_table_set.cpp @@ -139,7 +139,7 @@ void PostgresTableSet::LoadEntries(ClientContext &context) { table_result.reset(); } else { auto query = StringUtil::Replace(R"( - SELECT 0, relname, relpages, attname, + SELECT 0, relname, relkind, relpages, attname, pg_type.typname type_name, atttypmod type_modifier, pg_attribute.attndims ndim FROM pg_class JOIN pg_namespace ON relnamespace = pg_namespace.oid @@ -181,7 +181,7 @@ unique_ptr PostgresTableSet::GetTableInfo(PostgresTransactio return nullptr; } const idx_t COLUMN_OFFSET = 2; - auto relkind = result->GetString(0, 2); + auto relkind = result->GetString(0, 1); auto catalog_type = TransformRelKind(relkind); unique_ptr info; switch (catalog_type) { diff --git a/test/sql/storage/view/test_view_sql.test b/test/sql/storage/view/test_view_sql.test new file mode 100644 index 00000000..039f0d77 --- /dev/null +++ b/test/sql/storage/view/test_view_sql.test @@ -0,0 +1,64 @@ +# name: test/sql/catalog/view/test_view_sql.test +# description: Test behavior of 'sql' on various different views +# group: [view] + +require postgres_scanner + +require-env POSTGRES_TEST_DATABASE_AVAILABLE + +statement ok +PRAGMA enable_verification + +statement ok +ATTACH 'postgres:dbname=postgresscanner' AS s1 + +statement ok +drop schema if exists s1.my_schema cascade; + +statement ok +drop schema if exists s1."schema name" cascade; + +statement ok +drop view if exists s1.vw; + +statement ok +drop view if exists s1."view name"; + +statement ok +create schema s1.my_schema; + +# X contains columns `a` and `y` +statement ok +CREATE VIEW s1.my_schema.X (a) AS SELECT 'x' as x, 'y' as y; + +statement error +alter view s1.my_schema.x rename to Y; +---- +Binder Error: Only altering tables is supported for now + +statement ok +drop schema s1.my_schema cascade; + +statement ok +create view s1.vw as select * from VALUES (42, 'test') t(a, b); + +statement ok +create or replace view s1.vw (c1, c2) as select * from VALUES (42, 'test') t(a, b); + +statement ok +create or replace view s1."view name" as select * from VALUES (42, 'test') t(a, b); + +statement ok +drop view s1.vw; + +statement ok +drop view s1."view name" + +statement ok +create schema s1."schema name"; + +statement ok +CREATE VIEW s1."schema name"."view name" ( + "other name 1", + "column name 2" +) AS SELECT * FROM VALUES (42, 'test') t(a, b); From 326e6f6b26751b4d18e55c992474e3375ce0349a Mon Sep 17 00:00:00 2001 From: Tishj Date: Mon, 26 Feb 2024 18:12:14 +0100 Subject: [PATCH 08/12] fix issue caused by adding the relkind to the projection --- src/storage/postgres_table_set.cpp | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/src/storage/postgres_table_set.cpp b/src/storage/postgres_table_set.cpp index b14f6629..51fa1518 100644 --- a/src/storage/postgres_table_set.cpp +++ b/src/storage/postgres_table_set.cpp @@ -22,8 +22,15 @@ PostgresTableSet::PostgresTableSet(PostgresSchemaEntry &schema, unique_ptr PostgresTableSet::GetTableInfo(PostgresConnection if (rows == 0) { throw InvalidInputException("Table %s does not contain any columns.", table_name); } - auto relkind = result->GetString(0, 2); + + const idx_t COLUMN_OFFSET = 2; + auto relkind = result->GetString(0, 1); auto catalog_type = TransformRelKind(relkind); unique_ptr info; switch (catalog_type) { @@ -226,7 +240,7 @@ unique_ptr PostgresTableSet::GetTableInfo(PostgresConnection } } for (idx_t row = 0; row < rows; row++) { - AddColumn(nullptr, nullptr, *result, row, *info, 1); + AddColumn(nullptr, nullptr, *result, row, *info, COLUMN_OFFSET); } info->approx_num_pages = result->GetInt64(0, 0); return info; From 2be2eda643b2eaf9a91e1fa5beffda18b3d6d62f Mon Sep 17 00:00:00 2001 From: Tishj Date: Mon, 26 Feb 2024 18:16:03 +0100 Subject: [PATCH 09/12] revert changes to duckdb --- duckdb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/duckdb b/duckdb index 336c9be1..0e784765 160000 --- a/duckdb +++ b/duckdb @@ -1 +1 @@ -Subproject commit 336c9be1d2a7e419990d1d68634bc86f212d6b2b +Subproject commit 0e784765f6f87bd1ce9034afcce1e7f89fcd8777 From 392ad8e3e17b238da85511943fa117066fa409e4 Mon Sep 17 00:00:00 2001 From: Tishj Date: Tue, 27 Feb 2024 14:13:33 +0100 Subject: [PATCH 10/12] get the view definition, use it to make a select statement --- src/include/storage/postgres_view_entry.hpp | 9 ++++-- src/storage/postgres_table_set.cpp | 34 +++++++++++++++------ test/sql/storage/attach_views.test | 4 +-- 3 files changed, 32 insertions(+), 15 deletions(-) diff --git a/src/include/storage/postgres_view_entry.hpp b/src/include/storage/postgres_view_entry.hpp index f6b48bc3..d92e3056 100644 --- a/src/include/storage/postgres_view_entry.hpp +++ b/src/include/storage/postgres_view_entry.hpp @@ -16,16 +16,19 @@ namespace duckdb { struct PostgresViewInfo : public PostgresCreateInfo { public: - PostgresViewInfo() { + PostgresViewInfo(const string &select_stmt) { create_info = make_uniq(); + create_info->query = CreateViewInfo::ParseSelect(select_stmt); // create_info->columns.SetAllowDuplicates(true); } - PostgresViewInfo(const string &schema, const string &view) { + PostgresViewInfo(const string &schema, const string &view, const string &select_stmt) { create_info = make_uniq(string(), schema, view); + create_info->query = CreateViewInfo::ParseSelect(select_stmt); // create_info->columns.SetAllowDuplicates(true); } - PostgresViewInfo(const SchemaCatalogEntry &schema, const string &view) { + PostgresViewInfo(const SchemaCatalogEntry &schema, const string &view, const string &select_stmt) { create_info = make_uniq((SchemaCatalogEntry &)schema, view); + create_info->query = CreateViewInfo::ParseSelect(select_stmt); // create_info->columns.SetAllowDuplicates(true); } ~PostgresViewInfo() override { diff --git a/src/storage/postgres_table_set.cpp b/src/storage/postgres_table_set.cpp index 51fa1518..92a7c949 100644 --- a/src/storage/postgres_table_set.cpp +++ b/src/storage/postgres_table_set.cpp @@ -26,6 +26,7 @@ string PostgresTableSet::GetInitializeQuery() { pg_namespace.oid, relname, relkind, + pg_get_viewdef(pg_class.oid) AS view_definition, relpages, attname, pg_type.typname type_name, @@ -92,10 +93,11 @@ void PostgresTableSet::CreateEntries(PostgresTransaction &transaction, PostgresR unique_ptr info; for (idx_t row = start; row < end; row++) { - const idx_t COLUMN_OFFSET = 4; - auto relname = result.GetString(row, COLUMN_OFFSET - 3); + const idx_t COLUMN_OFFSET = 5; + auto relname = result.GetString(row, COLUMN_OFFSET - 4); D_ASSERT(!relname.empty()); - auto relkind = result.GetString(row, COLUMN_OFFSET - 2); + auto relkind = result.GetString(row, COLUMN_OFFSET - 3); + auto view_definition = result.GetString(row, COLUMN_OFFSET - 2); auto approx_num_pages = result.GetInt64(row, COLUMN_OFFSET - 1); if (!info || info->GetName() != relname) { if (info) { @@ -107,7 +109,7 @@ void PostgresTableSet::CreateEntries(PostgresTransaction &transaction, PostgresR info = make_uniq(schema, relname); break; case CatalogType::VIEW_ENTRY: - info = make_uniq(schema, relname); + info = make_uniq(schema, relname, view_definition); break; default: { throw InternalException("Unexpected CatalogType in CreateEntries: %s", @@ -142,12 +144,21 @@ void PostgresTableSet::CreateEntries(PostgresTransaction &transaction, PostgresR void PostgresTableSet::LoadEntries(ClientContext &context) { auto &transaction = PostgresTransaction::Get(context, catalog); if (table_result) { + // This query was created from PostgresTableSet::GetInitializeQuery() CreateEntries(transaction, table_result->GetResult(), table_result->start, table_result->end); table_result.reset(); } else { auto query = StringUtil::Replace(R"( - SELECT 0, relname, relkind, relpages, attname, - pg_type.typname type_name, atttypmod type_modifier, pg_attribute.attndims ndim + SELECT + 0 as "pg_namespace.oid", + relname, + relkind, + pg_get_viewdef(pg_class.oid) AS view_definition, + relpages, + attname, + pg_type.typname type_name, + atttypmod type_modifier, + pg_attribute.attndims ndim FROM pg_class JOIN pg_namespace ON relnamespace = pg_namespace.oid JOIN pg_attribute ON pg_class.oid=pg_attribute.attrelid @@ -169,6 +180,7 @@ string GetTableInfoQuery(const string &schema_name, const string &table_name) { SELECT relpages, relkind, + pg_get_viewdef(pg_class.oid) AS view_definition, attname, pg_type.typname type_name, atttypmod type_modifier, @@ -192,8 +204,9 @@ unique_ptr PostgresTableSet::GetTableInfo(PostgresTransactio if (rows == 0) { return nullptr; } - const idx_t COLUMN_OFFSET = 2; + const idx_t COLUMN_OFFSET = 3; auto relkind = result->GetString(0, 1); + auto view_definition = result->GetString(0, 2); auto catalog_type = TransformRelKind(relkind); unique_ptr info; switch (catalog_type) { @@ -201,7 +214,7 @@ unique_ptr PostgresTableSet::GetTableInfo(PostgresTransactio info = make_uniq(schema, table_name); break; case CatalogType::VIEW_ENTRY: - info = make_uniq(schema, table_name); + info = make_uniq(schema, table_name, view_definition); break; default: { throw InternalException("Unexpected CatalogType in GetTableInfo: %s", CatalogTypeToString(catalog_type)); @@ -224,8 +237,9 @@ unique_ptr PostgresTableSet::GetTableInfo(PostgresConnection throw InvalidInputException("Table %s does not contain any columns.", table_name); } - const idx_t COLUMN_OFFSET = 2; + const idx_t COLUMN_OFFSET = 3; auto relkind = result->GetString(0, 1); + auto view_definition = result->GetString(0, 2); auto catalog_type = TransformRelKind(relkind); unique_ptr info; switch (catalog_type) { @@ -233,7 +247,7 @@ unique_ptr PostgresTableSet::GetTableInfo(PostgresConnection info = make_uniq(schema_name, table_name); break; case CatalogType::VIEW_ENTRY: - info = make_uniq(schema_name, table_name); + info = make_uniq(schema_name, table_name, view_definition); break; default: { throw InternalException("Unexpected CatalogType in GetTableInfo: %s", CatalogTypeToString(catalog_type)); diff --git a/test/sql/storage/attach_views.test b/test/sql/storage/attach_views.test index b5145f5d..7c9b7d6e 100644 --- a/test/sql/storage/attach_views.test +++ b/test/sql/storage/attach_views.test @@ -34,7 +34,7 @@ already exists statement error INSERT INTO v1 VALUES (42); ---- -cannot copy to view +Catalog Error: v1 is not an table # FIXME - error message here is not very descriptive statement error @@ -48,7 +48,7 @@ DELETE FROM v1 statement error INSERT INTO v1 VALUES (1, 1); ---- -table v1 has 1 columns but 2 values were supplied +Catalog Error: v1 is not an table statement ok CREATE VIEW IF NOT EXISTS v1 AS SELECT 84; From 27ca89601a29bb4038ada196c665a90aff39f9c3 Mon Sep 17 00:00:00 2001 From: Tishj Date: Mon, 4 Mar 2024 10:10:08 +0100 Subject: [PATCH 11/12] rename from v1, suspecting naming conflict between tests --- test/sql/storage/attach_views.test | 42 +++++++++++++++--------------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/test/sql/storage/attach_views.test b/test/sql/storage/attach_views.test index 7c9b7d6e..31d431cc 100644 --- a/test/sql/storage/attach_views.test +++ b/test/sql/storage/attach_views.test @@ -16,74 +16,74 @@ statement ok USE s; statement ok -DROP VIEW IF EXISTS v1 +DROP VIEW IF EXISTS v5 statement ok -CREATE VIEW v1 AS SELECT 42 AS i; +CREATE VIEW v5 AS SELECT 42 AS i; query I -SELECT i FROM v1 +SELECT i FROM v5 ---- 42 statement error -CREATE VIEW v1 AS SELECT 84; +CREATE VIEW v5 AS SELECT 84; ---- already exists statement error -INSERT INTO v1 VALUES (42); +INSERT INTO v5 VALUES (42); ---- -Catalog Error: v1 is not an table +Catalog Error: v5 is not an table # FIXME - error message here is not very descriptive statement error -UPDATE v1 SET i=84 +UPDATE v5 SET i=84 ---- statement error -DELETE FROM v1 +DELETE FROM v5 ---- statement error -INSERT INTO v1 VALUES (1, 1); +INSERT INTO v5 VALUES (1, 1); ---- -Catalog Error: v1 is not an table +Catalog Error: v5 is not an table statement ok -CREATE VIEW IF NOT EXISTS v1 AS SELECT 84; +CREATE VIEW IF NOT EXISTS v5 AS SELECT 84; statement ok -CREATE OR REPLACE VIEW v1 AS SELECT 84; +CREATE OR REPLACE VIEW v5 AS SELECT 84; query I -SELECT * FROM v1 +SELECT * FROM v5 ---- 84 statement error -DROP TABLE v1 +DROP TABLE v5 ---- not a table statement ok -DROP VIEW v1 +DROP VIEW v5 statement error -SELECT * FROM v1 +SELECT * FROM v5 ---- -Table with name v1 does not exist +Table with name v5 does not exist statement error -DROP VIEW v1 +DROP VIEW v5 ---- -View with name v1 does not exist +View with name v5 does not exist statement ok -CREATE VIEW v1(a) AS SELECT 99; +CREATE VIEW v5(a) AS SELECT 99; query I -SELECT a FROM v1 +SELECT a FROM v5 ---- 99 From 91462b13dc8d97efb9813ebd7be2647487172724 Mon Sep 17 00:00:00 2001 From: Tishj Date: Sat, 23 Mar 2024 13:15:18 +0100 Subject: [PATCH 12/12] replace the '?column?' postgres names to avoid a binder exception --- src/storage/postgres_view_entry.cpp | 32 ++++++++- test/sql/storage/view/test_view_aliases.test | 69 ++++++++++++++++++++ 2 files changed, 99 insertions(+), 2 deletions(-) create mode 100644 test/sql/storage/view/test_view_aliases.test diff --git a/src/storage/postgres_view_entry.cpp b/src/storage/postgres_view_entry.cpp index cfd6659f..21729812 100644 --- a/src/storage/postgres_view_entry.cpp +++ b/src/storage/postgres_view_entry.cpp @@ -3,6 +3,7 @@ #include "storage/postgres_transaction.hpp" #include "duckdb/storage/statistics/base_statistics.hpp" #include "duckdb/catalog/catalog_entry/view_catalog_entry.hpp" +#include "duckdb/parser/query_node/select_node.hpp" #include "postgres_scanner.hpp" namespace duckdb { @@ -22,9 +23,36 @@ PostgresViewEntry::PostgresViewEntry(Catalog &catalog, SchemaCatalogEntry &schem approx_num_pages = 0; } +static CreateViewInfo &ReplaceUnknownNames(CreateViewInfo &view_info) { + // When expressions like `select 'x'` are passed to postgres, the "?column?" name is given to them + // this clashes with DuckDB's behavior, as we assign the ToString of the expression to them instead. + // so we have to replace the "?column?" names with their DuckDB version + auto &select_node = view_info.query->node->Cast(); + auto &select_list = select_node.select_list; + // Aliases can override names, if provided + D_ASSERT(view_info.aliases.size() <= view_info.names.size()); + // Every name corresponds to an expression + D_ASSERT(view_info.names.size() == select_list.size()); + for (idx_t i = 0; i < view_info.names.size(); i++) { + if (i < view_info.aliases.size()) { + // No need to do anything, alias will be used instead of the expression name + continue; + } + if (view_info.names[i] != "?column?") { + // This expression was given an alias already + continue; + } + auto &expression = select_list[i]; + // If it had an alias the postgres name wouldn't be "?column?" + D_ASSERT(expression->alias.empty()); + view_info.names[i] = expression->ToString(); + } + return view_info; +} + PostgresViewEntry::PostgresViewEntry(Catalog &catalog, SchemaCatalogEntry &schema, PostgresViewInfo &info) - : ViewCatalogEntry(catalog, schema, *info.create_info), postgres_types(std::move(info.postgres_types)), - postgres_names(std::move(info.postgres_names)) { + : ViewCatalogEntry(catalog, schema, ReplaceUnknownNames(*info.create_info)), + postgres_types(std::move(info.postgres_types)), postgres_names(std::move(info.postgres_names)) { D_ASSERT(postgres_types.size() == types.size()); approx_num_pages = info.approx_num_pages; } diff --git a/test/sql/storage/view/test_view_aliases.test b/test/sql/storage/view/test_view_aliases.test new file mode 100644 index 00000000..64ad693d --- /dev/null +++ b/test/sql/storage/view/test_view_aliases.test @@ -0,0 +1,69 @@ +# name: test/sql/catalog/view/test_view_aliases.test +# description: Test behavior of aliasing on view creation +# group: [view] + +require postgres_scanner + +require-env POSTGRES_TEST_DATABASE_AVAILABLE + +statement ok +PRAGMA enable_verification + +statement ok +ATTACH 'postgres:dbname=postgresscanner' AS s1 + +statement ok +drop schema if exists s1.view_aliases cascade; + +statement ok +create schema s1.view_aliases; + +# Fully aliased +statement ok +CREATE VIEW s1.view_aliases.X (a) AS SELECT 'x' + +query I +select a from s1.view_aliases.x; +---- +x + +statement ok +drop view s1.view_aliases.x; + +# Not aliased (no expression alias) +statement ok +CREATE VIEW s1.view_aliases.x AS SELECT 'x' + +query I +select x from s1.view_aliases.x +---- +{'CAST('x' AS VARCHAR)': x} + +statement ok +drop view s1.view_aliases.x; + +# Not aliased (with expression alias) +statement ok +CREATE VIEW s1.view_aliases.x AS SELECT 'x' x + +query I +select x from s1.view_aliases.x +---- +x + +statement ok +drop view s1.view_aliases.x; + +# Partially aliased +statement error +CREATE VIEW s1.view_aliases.x (x) AS SELECT 'a', 'x' x +---- +ERROR: column "x" specified more than once + +statement ok +CREATE VIEW s1.view_aliases.x (x) AS SELECT 'a', 'x' also_x + +query I +select x from s1.view_aliases.x +---- +a