adnum logical error on postgres table SELECT query

[ischepin]

Describe what's wrong
Clickhouse cannot query postgres tables with autogenerated columns that have been altered
Looks like its coming from #57568

Does it reproduce on the most recent release?
Yes, reproducible on 24.3 and head-alpine docker image.

How to reproduce
The setup:
Clickhouse instance connected to postgres instance, using pg_conn named collection for creds

Postgres table:

db=# create table test (id integer primary key);
CREATE TABLE
db=# alter table test add column val integer generated always as (id + 1) stored;
ALTER TABLE
db=# alter table test drop column val;
ALTER TABLE
db=# alter table test add column val integer generated always as (id + 2) stored;
ALTER TABLE
db=# select attname,attnum from pg_attribute
where attrelid = 'test'::regclass and attnum > 0
order by attnum;
           attname            | attnum
------------------------------+--------
 id                           |      1
 ........pg.dropped.2........ |      2
 val                          |      3
(3 rows)

Clickhouse query:

ba443a2d339a :) select * from postgresql(pg_conn, table='test');

SELECT *
FROM postgresql(pg_conn, `table` = 'test')

Query id: f70d8b00-55e6-4255-8ce8-822a89e93c18

Received exception from server (version 24.3.2):
Code: 49. DB::Exception: Received from localhost:9000. DB::Exception: Received adnum 3, but currently fetched columns list has 2 columns. (LOGICAL_ERROR)

Expected behavior

Table 'test' can be queried from clickhouse

Error message and/or stacktrace
Stacktrace

0. DB::Exception::Exception(DB::Exception::MessageMasked&&, int, bool) @ 0x000000000bf4e0a8
1. DB::Exception::Exception<unsigned long&, unsigned long>(int, FormatStringHelperImpl<std::type_identity<unsigned long&>::type, std::type_identity<unsigned long>::type>, unsigned long&, unsigned long&&) @ 0x00000000077cd0a4
2. DB::PostgreSQLTableStructure DB::fetchPostgreSQLTableStructure<pqxx::transaction<(pqxx::isolation_level)0, (pqxx::write_policy)0>>(pqxx::transaction<(pqxx::isolation_level)0, (pqxx::write_policy)0>&, String const&, String const&, bool, bool, bool) @ 0x000000000ef73cc4
3. DB::fetchPostgreSQLTableStructure(pqxx::connection&, String const&, String const&, bool) @ 0x000000000ef7f788
4. DB::StoragePostgreSQL::getTableStructureFromData(std::shared_ptr<postgres::PoolWithFailover> const&, String const&, String const&, std::shared_ptr<DB::Context const> const&) @ 0x00000000101e65fc
5. DB::StoragePostgreSQL::StoragePostgreSQL(DB::StorageID const&, std::shared_ptr<postgres::PoolWithFailover>, String const&, DB::ColumnsDescription const&, DB::ConstraintsDescription const&, String const&, std::shared_ptr<DB::Context const>, String const&, String const&) @ 0x00000000101e6154
6. DB::(anonymous namespace)::TableFunctionPostgreSQL::executeImpl(std::shared_ptr<DB::IAST> const&, std::shared_ptr<DB::Context const>, String const&, DB::ColumnsDescription, bool) const @ 0x000000000e459054
7. DB::ITableFunction::execute(std::shared_ptr<DB::IAST> const&, std::shared_ptr<DB::Context const>, String const&, DB::ColumnsDescription, bool, bool) const @ 0x000000000e6c55ec
8. DB::Context::executeTableFunction(std::shared_ptr<DB::IAST> const&, std::shared_ptr<DB::ITableFunction> const&) @ 0x000000000f18af90
9. DB::(anonymous namespace)::QueryAnalyzer::resolveTableFunction(std::shared_ptr<DB::IQueryTreeNode>&, DB::(anonymous namespace)::IdentifierResolveScope&, DB::(anonymous namespace)::QueryExpressionsAliasVisitor&, bool) @ 0x000000000f77ed6c
10. DB::(anonymous namespace)::QueryAnalyzer::resolveQueryJoinTreeNode(std::shared_ptr<DB::IQueryTreeNode>&, DB::(anonymous namespace)::IdentifierResolveScope&, DB::(anonymous namespace)::QueryExpressionsAliasVisitor&) @ 0x000000000f78149c
11. DB::(anonymous namespace)::QueryAnalyzer::resolveQuery(std::shared_ptr<DB::IQueryTreeNode> const&, DB::(anonymous namespace)::IdentifierResolveScope&) @ 0x000000000f772eb8
12. DB::QueryAnalysisPass::run(std::shared_ptr<DB::IQueryTreeNode>&, std::shared_ptr<DB::Context const>) @ 0x000000000f77102c
13. DB::QueryTreePassManager::run(std::shared_ptr<DB::IQueryTreeNode>) @ 0x000000000f76f8a4
14. DB::(anonymous namespace)::buildQueryTreeAndRunPasses(std::shared_ptr<DB::IAST> const&, DB::SelectQueryOptions const&, std::shared_ptr<DB::Context const> const&, std::shared_ptr<DB::IStorage> const&) (.llvm.6419921672193001469) @ 0x000000000f99fce4
15. DB::InterpreterSelectQueryAnalyzer::InterpreterSelectQueryAnalyzer(std::shared_ptr<DB::IAST> const&, std::shared_ptr<DB::Context const> const&, DB::SelectQueryOptions const&) @ 0x000000000f99eb78
16. std::unique_ptr<DB::IInterpreter, std::default_delete<DB::IInterpreter>> std::__function::__policy_invoker<std::unique_ptr<DB::IInterpreter, std::default_delete<DB::IInterpreter>> (DB::InterpreterFactory::Arguments const&)>::__call_impl<std::__function::__default_alloc_func<DB::registerInterpreterSelectQueryAnalyzer(DB::InterpreterFactory&)::$_0, std::unique_ptr<DB::IInterpreter, std::default_delete<DB::IInterpreter>> (DB::InterpreterFactory::Arguments const&)>>(std::__function::__policy_storage const*, DB::InterpreterFactory::Arguments const&) (.llvm.6419921672193001469) @ 0x000000000f9a179c
17. DB::InterpreterFactory::get(std::shared_ptr<DB::IAST>&, std::shared_ptr<DB::Context>, DB::SelectQueryOptions const&) @ 0x000000000f943850
18. DB::executeQueryImpl(char const*, char const*, std::shared_ptr<DB::Context>, DB::QueryFlags, DB::QueryProcessingStage::Enum, DB::ReadBuffer*) @ 0x000000000fd0ed78
19. DB::executeQuery(String const&, std::shared_ptr<DB::Context>, DB::QueryFlags, DB::QueryProcessingStage::Enum) @ 0x000000000fd0b6d0
20. DB::TCPHandler::runImpl() @ 0x0000000010b7e7bc
21. DB::TCPHandler::run() @ 0x0000000010b955a8
22. Poco::Net::TCPServerConnection::start() @ 0x0000000012f1ef44
23. Poco::Net::TCPServerDispatcher::run() @ 0x0000000012f1ff10
24. Poco::PooledThread::run() @ 0x000000001303fbf4
25. Poco::ThreadImpl::runnableEntry(void*) @ 0x000000001303dfac
26. start_thread @ 0x0000000000007624
27. ? @ 0x00000000000d162c

[JP-Reddy]

I'm able to repro this. It seems any alter operation on the columns of the table will add an entry to the pg_attribute table. Whenever we're trying to fetch data from pg_attribute related to the table, we get data of all the columns that were deleted too. And that doesn't match with the actual number of columns present in the table, and an exception is being raised[1].

One solution would be to ignore all such additional rows that were added during deletes("........pg.dropped.2........") instead of throwing an exception. @Algunenano thoughts on how this should be addressed? I can help fix this.

[1]

if (check_generated)
{
    std::string attrdef_query = fmt::format(
        "SELECT adnum, pg_get_expr(adbin, adrelid) as generated_expression "
        "FROM pg_attrdef "
        "WHERE adrelid = (SELECT oid FROM pg_class WHERE {});", where);

    pqxx::result result{tx.exec(attrdef_query)};
    for (const auto row : result)
    {
        size_t adnum = row[0].as<int>();
        if (!adnum || adnum > table.physical_columns->names.size())
        {
            throw Exception(ErrorCodes::LOGICAL_ERROR,
                            "Received adnum {}, but currently fetched columns list has {} columns",
                            adnum, table.physical_columns->attributes.size());
        }
        const auto column_name = table.physical_columns->names[adnum - 1];
        table.physical_columns->attributes.at(column_name).attr_def = row[1].as<std::string>();
    }
}

[JP-Reddy]

@Algunenano can I go ahead ahead with the approach I mentioned?

[Algunenano]

It makes sense to ignore columns that are dropped. I guess that we need to do it via SQL pg_attribute.attisdropped but I haven't tested it to confirm how it works.