From a223526f451d2bdb32041a137ca9f5bbf31dcc28 Mon Sep 17 00:00:00 2001 From: kssenii Date: Wed, 20 Oct 2021 23:29:35 +0000 Subject: [PATCH 1/5] Fix --- src/Databases/PostgreSQL/fetchPostgreSQLTableStructure.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Databases/PostgreSQL/fetchPostgreSQLTableStructure.cpp b/src/Databases/PostgreSQL/fetchPostgreSQLTableStructure.cpp index 5ac4180ec27..2adb359eae5 100644 --- a/src/Databases/PostgreSQL/fetchPostgreSQLTableStructure.cpp +++ b/src/Databases/PostgreSQL/fetchPostgreSQLTableStructure.cpp @@ -216,7 +216,7 @@ PostgreSQLTableStructure fetchPostgreSQLTableStructure( "SELECT attname AS name, format_type(atttypid, atttypmod) AS type, " "attnotnull AS not_null, attndims AS dims " "FROM pg_attribute " - "WHERE attrelid = {}::regclass " + "WHERE attrelid = (SELECT oid FROM pg_class WHERE relname = {})" "AND NOT attisdropped AND attnum > 0", quoteString(postgres_table_name)); table.columns = readNamesAndTypesList(tx, postgres_table_name, query, use_nulls, false); From c95a29edacf6d410b794c8663a0511711ec1191e Mon Sep 17 00:00:00 2001 From: kssenii Date: Fri, 22 Oct 2021 08:16:42 +0000 Subject: [PATCH 2/5] Fix quoting --- .../PostgreSQL/DatabasePostgreSQL.cpp | 9 ++-- src/Databases/PostgreSQL/DatabasePostgreSQL.h | 2 +- .../fetchPostgreSQLTableStructure.cpp | 42 ++++++++++--------- .../fetchPostgreSQLTableStructure.h | 4 +- .../PostgreSQLReplicationHandler.cpp | 4 +- .../TableFunctionPostgreSQL.cpp | 7 ++-- 6 files changed, 36 insertions(+), 32 deletions(-) diff --git a/src/Databases/PostgreSQL/DatabasePostgreSQL.cpp b/src/Databases/PostgreSQL/DatabasePostgreSQL.cpp index 430caa0f4a3..d671a2b87d7 100644 --- a/src/Databases/PostgreSQL/DatabasePostgreSQL.cpp +++ b/src/Databases/PostgreSQL/DatabasePostgreSQL.cpp @@ -63,11 +63,12 @@ String DatabasePostgreSQL::getTableNameForLogs(const String & table_name) const } -String DatabasePostgreSQL::formatTableName(const String & table_name) const +String DatabasePostgreSQL::formatTableName(const String & table_name, bool quoted) const { if (configuration.schema.empty()) - return doubleQuoteString(table_name); - return fmt::format("{}.{}", doubleQuoteString(configuration.schema), doubleQuoteString(table_name)); + return quoted ? doubleQuoteString(table_name) : table_name; + return quoted ? fmt::format("{}.{}", doubleQuoteString(configuration.schema), doubleQuoteString(table_name)) + : fmt::format("{}.{}", configuration.schema, table_name); } @@ -170,7 +171,7 @@ StoragePtr DatabasePostgreSQL::fetchTable(const String & table_name, ContextPtr, return StoragePtr{}; auto connection_holder = pool->get(); - auto columns = fetchPostgreSQLTableStructure(connection_holder->get(), formatTableName(table_name)).columns; + auto columns = fetchPostgreSQLTableStructure(connection_holder->get(), table_name, configuration.schema).columns; if (!columns) return StoragePtr{}; diff --git a/src/Databases/PostgreSQL/DatabasePostgreSQL.h b/src/Databases/PostgreSQL/DatabasePostgreSQL.h index 0f66a6c7b90..c64feb74020 100644 --- a/src/Databases/PostgreSQL/DatabasePostgreSQL.h +++ b/src/Databases/PostgreSQL/DatabasePostgreSQL.h @@ -79,7 +79,7 @@ private: String getTableNameForLogs(const String & table_name) const; - String formatTableName(const String & table_name) const; + String formatTableName(const String & table_name, bool quoted = true) const; bool checkPostgresTable(const String & table_name) const; diff --git a/src/Databases/PostgreSQL/fetchPostgreSQLTableStructure.cpp b/src/Databases/PostgreSQL/fetchPostgreSQLTableStructure.cpp index 2adb359eae5..fd6b69e39f9 100644 --- a/src/Databases/PostgreSQL/fetchPostgreSQLTableStructure.cpp +++ b/src/Databases/PostgreSQL/fetchPostgreSQLTableStructure.cpp @@ -135,7 +135,7 @@ static DataTypePtr convertPostgreSQLDataType(String & type, Fn auto && r template std::shared_ptr readNamesAndTypesList( - T & tx, const String & postgres_table_name, const String & query, bool use_nulls, bool only_names_and_types) + T & tx, const String & postgres_table, const String & query, bool use_nulls, bool only_names_and_types) { auto columns = NamesAndTypes(); @@ -180,7 +180,7 @@ std::shared_ptr readNamesAndTypesList( /// All rows must contain the same number of dimensions, so limit 1 is ok. If number of dimensions in all rows is not the same - /// such arrays are not able to be used as ClickHouse Array at all. - pqxx::result result{tx.exec(fmt::format("SELECT array_ndims({}) FROM {} LIMIT 1", name_and_type.name, postgres_table_name))}; + pqxx::result result{tx.exec(fmt::format("SELECT array_ndims({}) FROM {} LIMIT 1", name_and_type.name, postgres_table))}; auto dimensions = result[0][0].as(); /// It is always 1d array if it is in recheck. @@ -194,7 +194,7 @@ std::shared_ptr readNamesAndTypesList( catch (const pqxx::undefined_table &) { - throw Exception(ErrorCodes::UNKNOWN_TABLE, "PostgreSQL table {} does not exist", postgres_table_name); + throw Exception(ErrorCodes::UNKNOWN_TABLE, "PostgreSQL table {} does not exist", postgres_table); } catch (Exception & e) { @@ -208,18 +208,22 @@ std::shared_ptr readNamesAndTypesList( template PostgreSQLTableStructure fetchPostgreSQLTableStructure( - T & tx, const String & postgres_table_name, bool use_nulls, bool with_primary_key, bool with_replica_identity_index) + T & tx, const String & postgres_table, const String & postgres_schema, bool use_nulls, bool with_primary_key, bool with_replica_identity_index) { PostgreSQLTableStructure table; + auto where = fmt::format("relname = {}", quoteString(postgres_table)); + if (!postgres_schema.empty()) + where += fmt::format(" AND relnamespace = (SELECT oid FROM pg_namespace WHERE nspname = {})", quoteString(postgres_schema)); + std::string query = fmt::format( "SELECT attname AS name, format_type(atttypid, atttypmod) AS type, " "attnotnull AS not_null, attndims AS dims " "FROM pg_attribute " - "WHERE attrelid = (SELECT oid FROM pg_class WHERE relname = {})" - "AND NOT attisdropped AND attnum > 0", quoteString(postgres_table_name)); + "WHERE attrelid = (SELECT oid FROM pg_class WHERE {}) " + "AND NOT attisdropped AND attnum > 0", where); - table.columns = readNamesAndTypesList(tx, postgres_table_name, query, use_nulls, false); + table.columns = readNamesAndTypesList(tx, postgres_table, query, use_nulls, false); if (with_primary_key) { @@ -229,9 +233,9 @@ PostgreSQLTableStructure fetchPostgreSQLTableStructure( "FROM pg_index i " "JOIN pg_attribute a ON a.attrelid = i.indrelid " "AND a.attnum = ANY(i.indkey) " - "WHERE i.indrelid = {}::regclass AND i.indisprimary", quoteString(postgres_table_name)); + "WHERE attrelid = (SELECT oid FROM pg_class WHERE {}) AND i.indisprimary", where); - table.primary_key_columns = readNamesAndTypesList(tx, postgres_table_name, query, use_nulls, true); + table.primary_key_columns = readNamesAndTypesList(tx, postgres_table, query, use_nulls, true); } if (with_replica_identity_index && !table.primary_key_columns) @@ -254,19 +258,19 @@ PostgreSQLTableStructure fetchPostgreSQLTableStructure( "and t.relname = {} " /// Connection is already done to a needed database, only table name is needed. "and ix.indisreplident = 't' " /// index is is replica identity index "ORDER BY a.attname", /// column names - quoteString(postgres_table_name)); + quoteString(postgres_table)); - table.replica_identity_columns = readNamesAndTypesList(tx, postgres_table_name, query, use_nulls, true); + table.replica_identity_columns = readNamesAndTypesList(tx, postgres_table, query, use_nulls, true); } return table; } -PostgreSQLTableStructure fetchPostgreSQLTableStructure(pqxx::connection & connection, const String & postgres_table_name, bool use_nulls) +PostgreSQLTableStructure fetchPostgreSQLTableStructure(pqxx::connection & connection, const String & postgres_table, const String & postgres_schema, bool use_nulls) { pqxx::ReadTransaction tx(connection); - auto result = fetchPostgreSQLTableStructure(tx, postgres_table_name, use_nulls, false, false); + auto result = fetchPostgreSQLTableStructure(tx, postgres_table, postgres_schema, use_nulls, false, false); tx.commit(); return result; } @@ -283,18 +287,18 @@ std::set fetchPostgreSQLTablesList(pqxx::connection & connection, const template PostgreSQLTableStructure fetchPostgreSQLTableStructure( - pqxx::ReadTransaction & tx, const String & postgres_table_name, bool use_nulls, - bool with_primary_key, bool with_replica_identity_index); + pqxx::ReadTransaction & tx, const String & postgres_table, const String & postgres_schema, + bool use_nulls, bool with_primary_key, bool with_replica_identity_index); template PostgreSQLTableStructure fetchPostgreSQLTableStructure( - pqxx::ReplicationTransaction & tx, const String & postgres_table_name, bool use_nulls, - bool with_primary_key, bool with_replica_identity_index); + pqxx::ReplicationTransaction & tx, const String & postgres_table, const String & postgres_schema, + bool use_nulls, bool with_primary_key, bool with_replica_identity_index); template PostgreSQLTableStructure fetchPostgreSQLTableStructure( - pqxx::nontransaction & tx, const String & postgres_table_name, bool use_nulls, - bool with_primary_key, bool with_replica_identity_index); + pqxx::nontransaction & tx, const String & postgres_table, const String & postrges_schema, + bool use_nulls, bool with_primary_key, bool with_replica_identity_index); template std::set fetchPostgreSQLTablesList(pqxx::work & tx, const String & postgres_schema); diff --git a/src/Databases/PostgreSQL/fetchPostgreSQLTableStructure.h b/src/Databases/PostgreSQL/fetchPostgreSQLTableStructure.h index 62f85e7f414..809378abe38 100644 --- a/src/Databases/PostgreSQL/fetchPostgreSQLTableStructure.h +++ b/src/Databases/PostgreSQL/fetchPostgreSQLTableStructure.h @@ -24,11 +24,11 @@ using PostgreSQLTableStructurePtr = std::unique_ptr; std::set fetchPostgreSQLTablesList(pqxx::connection & connection, const String & postgres_schema); PostgreSQLTableStructure fetchPostgreSQLTableStructure( - pqxx::connection & connection, const String & postgres_table_name, bool use_nulls = true); + pqxx::connection & connection, const String & postgres_table, const String & postgres_schema, bool use_nulls = true); template PostgreSQLTableStructure fetchPostgreSQLTableStructure( - T & tx, const String & postgres_table_name, bool use_nulls = true, + T & tx, const String & postgres_table, const String & postgres_schema, bool use_nulls = true, bool with_primary_key = false, bool with_replica_identity_index = false); template diff --git a/src/Storages/PostgreSQL/PostgreSQLReplicationHandler.cpp b/src/Storages/PostgreSQL/PostgreSQLReplicationHandler.cpp index 3796bd8ba57..3262a302f9f 100644 --- a/src/Storages/PostgreSQL/PostgreSQLReplicationHandler.cpp +++ b/src/Storages/PostgreSQL/PostgreSQLReplicationHandler.cpp @@ -229,7 +229,7 @@ ASTPtr PostgreSQLReplicationHandler::getCreateNestedTableQuery(StorageMaterializ { postgres::Connection connection(connection_info); pqxx::nontransaction tx(connection.getRef()); - auto table_structure = std::make_unique(fetchPostgreSQLTableStructure(tx, table_name, true, true, true)); + auto table_structure = std::make_unique(fetchPostgreSQLTableStructure(tx, table_name, postgres_schema, true, true, true)); return storage->getCreateNestedTableQuery(std::move(table_structure)); } @@ -649,7 +649,7 @@ PostgreSQLTableStructurePtr PostgreSQLReplicationHandler::fetchTableStructure( if (!is_materialized_postgresql_database) return nullptr; - return std::make_unique(fetchPostgreSQLTableStructure(tx, table_name, true, true, true)); + return std::make_unique(fetchPostgreSQLTableStructure(tx, table_name, postgres_schema, true, true, true)); } diff --git a/src/TableFunctions/TableFunctionPostgreSQL.cpp b/src/TableFunctions/TableFunctionPostgreSQL.cpp index 980066622c8..1c0b78925e2 100644 --- a/src/TableFunctions/TableFunctionPostgreSQL.cpp +++ b/src/TableFunctions/TableFunctionPostgreSQL.cpp @@ -49,11 +49,10 @@ ColumnsDescription TableFunctionPostgreSQL::getActualTableStructure(ContextPtr c const bool use_nulls = context->getSettingsRef().external_table_functions_use_nulls; auto connection_holder = connection_pool->get(); auto columns = fetchPostgreSQLTableStructure( - connection_holder->get(), - configuration->schema.empty() ? doubleQuoteString(configuration->table) - : doubleQuoteString(configuration->schema) + '.' + doubleQuoteString(configuration->table), - use_nulls).columns; + connection_holder->get(), configuration->table, configuration->schema, use_nulls).columns; + if (!columns) + throw Exception(ErrorCodes::BAD_ARGUMENTS, "Table strcuture not returned"); return ColumnsDescription{*columns}; } From 057fe51ec85d1974028abe390196a2e2ac91199f Mon Sep 17 00:00:00 2001 From: Kseniia Sumarokova <54203879+kssenii@users.noreply.github.com> Date: Fri, 22 Oct 2021 13:08:38 +0300 Subject: [PATCH 3/5] Update TableFunctionPostgreSQL.cpp --- src/TableFunctions/TableFunctionPostgreSQL.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/TableFunctions/TableFunctionPostgreSQL.cpp b/src/TableFunctions/TableFunctionPostgreSQL.cpp index 1c0b78925e2..d929c62237c 100644 --- a/src/TableFunctions/TableFunctionPostgreSQL.cpp +++ b/src/TableFunctions/TableFunctionPostgreSQL.cpp @@ -52,7 +52,7 @@ ColumnsDescription TableFunctionPostgreSQL::getActualTableStructure(ContextPtr c connection_holder->get(), configuration->table, configuration->schema, use_nulls).columns; if (!columns) - throw Exception(ErrorCodes::BAD_ARGUMENTS, "Table strcuture not returned"); + throw Exception(ErrorCodes::BAD_ARGUMENTS, "Table structure not returned"); return ColumnsDescription{*columns}; } From 1fa123ee5c0a535d56eb0d12ea769a0409c0322a Mon Sep 17 00:00:00 2001 From: kssenii Date: Sat, 23 Oct 2021 01:33:17 +0300 Subject: [PATCH 4/5] Properly done --- .../PostgreSQL/DatabasePostgreSQL.cpp | 21 +++++++++++++------ .../fetchPostgreSQLTableStructure.cpp | 12 +++++++++-- .../PostgreSQLReplicationHandler.cpp | 14 ++++++++++++- 3 files changed, 38 insertions(+), 9 deletions(-) diff --git a/src/Databases/PostgreSQL/DatabasePostgreSQL.cpp b/src/Databases/PostgreSQL/DatabasePostgreSQL.cpp index d671a2b87d7..5d3493d0c82 100644 --- a/src/Databases/PostgreSQL/DatabasePostgreSQL.cpp +++ b/src/Databases/PostgreSQL/DatabasePostgreSQL.cpp @@ -90,14 +90,23 @@ bool DatabasePostgreSQL::empty() const DatabaseTablesIteratorPtr DatabasePostgreSQL::getTablesIterator(ContextPtr local_context, const FilterByNameFunction & /* filter_by_table_name */) const { std::lock_guard lock(mutex); - Tables tables; - auto connection_holder = pool->get(); - auto table_names = fetchPostgreSQLTablesList(connection_holder->get(), configuration.schema); - for (const auto & table_name : table_names) - if (!detached_or_dropped.count(table_name)) - tables[table_name] = fetchTable(table_name, local_context, true); + /// Do not allow to throw here, because this might be, for example, a query to system.tables. + /// It must not fail on case of some postgres error. + try + { + auto connection_holder = pool->get(); + auto table_names = fetchPostgreSQLTablesList(connection_holder->get(), configuration.schema); + + for (const auto & table_name : table_names) + if (!detached_or_dropped.count(table_name)) + tables[table_name] = fetchTable(table_name, local_context, true); + } + catch (...) + { + tryLogCurrentException(__PRETTY_FUNCTION__); + } return std::make_unique(tables, database_name); } diff --git a/src/Databases/PostgreSQL/fetchPostgreSQLTableStructure.cpp b/src/Databases/PostgreSQL/fetchPostgreSQLTableStructure.cpp index fd6b69e39f9..feb472fa152 100644 --- a/src/Databases/PostgreSQL/fetchPostgreSQLTableStructure.cpp +++ b/src/Databases/PostgreSQL/fetchPostgreSQLTableStructure.cpp @@ -191,7 +191,10 @@ std::shared_ptr readNamesAndTypesList( columns[i] = NameAndTypePair(name_and_type.name, type); } } - + catch (const pqxx::syntax_error & e) + { + throw Exception(ErrorCodes::BAD_ARGUMENTS, "Error: {} (in query: {})", e.what(), query); + } catch (const pqxx::undefined_table &) { throw Exception(ErrorCodes::UNKNOWN_TABLE, "PostgreSQL table {} does not exist", postgres_table); @@ -213,7 +216,9 @@ PostgreSQLTableStructure fetchPostgreSQLTableStructure( PostgreSQLTableStructure table; auto where = fmt::format("relname = {}", quoteString(postgres_table)); - if (!postgres_schema.empty()) + if (postgres_schema.empty()) + where += " AND relnamespace = (SELECT oid FROM pg_namespace WHERE nspname = 'public')"; + else where += fmt::format(" AND relnamespace = (SELECT oid FROM pg_namespace WHERE nspname = {})", quoteString(postgres_schema)); std::string query = fmt::format( @@ -225,6 +230,9 @@ PostgreSQLTableStructure fetchPostgreSQLTableStructure( table.columns = readNamesAndTypesList(tx, postgres_table, query, use_nulls, false); + if (!table.columns) + throw Exception(ErrorCodes::UNKNOWN_TABLE, "PostgreSQL table {} does not exist", postgres_table); + if (with_primary_key) { /// wiki.postgresql.org/wiki/Retrieve_primary_key_columns diff --git a/src/Storages/PostgreSQL/PostgreSQLReplicationHandler.cpp b/src/Storages/PostgreSQL/PostgreSQLReplicationHandler.cpp index 3262a302f9f..670148e9baa 100644 --- a/src/Storages/PostgreSQL/PostgreSQLReplicationHandler.cpp +++ b/src/Storages/PostgreSQL/PostgreSQLReplicationHandler.cpp @@ -230,6 +230,8 @@ ASTPtr PostgreSQLReplicationHandler::getCreateNestedTableQuery(StorageMaterializ postgres::Connection connection(connection_info); pqxx::nontransaction tx(connection.getRef()); auto table_structure = std::make_unique(fetchPostgreSQLTableStructure(tx, table_name, postgres_schema, true, true, true)); + if (!table_structure) + throw Exception(ErrorCodes::LOGICAL_ERROR, "Failed to get PostgreSQL table structure"); return storage->getCreateNestedTableQuery(std::move(table_structure)); } @@ -649,7 +651,17 @@ PostgreSQLTableStructurePtr PostgreSQLReplicationHandler::fetchTableStructure( if (!is_materialized_postgresql_database) return nullptr; - return std::make_unique(fetchPostgreSQLTableStructure(tx, table_name, postgres_schema, true, true, true)); + PostgreSQLTableStructure structure; + try + { + structure = fetchPostgreSQLTableStructure(tx, table_name, postgres_schema, true, true, true); + } + catch (...) + { + tryLogCurrentException(__PRETTY_FUNCTION__); + } + + return std::make_unique(std::move(structure)); } From f362d420b8e78154752c57ed1441e3af26eb5966 Mon Sep 17 00:00:00 2001 From: Kseniia Sumarokova <54203879+kssenii@users.noreply.github.com> Date: Sat, 23 Oct 2021 11:38:18 +0300 Subject: [PATCH 5/5] Update fetchPostgreSQLTableStructure.cpp --- .../PostgreSQL/fetchPostgreSQLTableStructure.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Databases/PostgreSQL/fetchPostgreSQLTableStructure.cpp b/src/Databases/PostgreSQL/fetchPostgreSQLTableStructure.cpp index feb472fa152..16e7ce89579 100644 --- a/src/Databases/PostgreSQL/fetchPostgreSQLTableStructure.cpp +++ b/src/Databases/PostgreSQL/fetchPostgreSQLTableStructure.cpp @@ -191,14 +191,14 @@ std::shared_ptr readNamesAndTypesList( columns[i] = NameAndTypePair(name_and_type.name, type); } } - catch (const pqxx::syntax_error & e) - { - throw Exception(ErrorCodes::BAD_ARGUMENTS, "Error: {} (in query: {})", e.what(), query); - } catch (const pqxx::undefined_table &) { throw Exception(ErrorCodes::UNKNOWN_TABLE, "PostgreSQL table {} does not exist", postgres_table); } + catch (const pqxx::syntax_error & e) + { + throw Exception(ErrorCodes::BAD_ARGUMENTS, "Error: {} (in query: {})", e.what(), query); + } catch (Exception & e) { e.addMessage("while fetching postgresql table structure");