Skip to content

Antalya 26.1 Backport of #97062 - Improve catalog show tables query#1552

Open
mkmkme wants to merge 2 commits intoantalya-26.1from
backports/antalya-26.1/97062
Open

Antalya 26.1 Backport of #97062 - Improve catalog show tables query#1552
mkmkme wants to merge 2 commits intoantalya-26.1from
backports/antalya-26.1/97062

Conversation

@mkmkme
Copy link
Collaborator

@mkmkme mkmkme commented Mar 19, 2026

Revert "Revert "Improve catalog show tables query""

Changelog category (leave one):

  • Improvement

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Improved processing 'show tables' query by fetching only names of tables and improved getLightweightTablesIterator to return structure containing only table names (ClickHouse#97062 by @SmitaRKulkarni)

CI/CD Options

Exclude tests:

  • Fast test
  • Integration Tests
  • Stateless tests
  • Stateful tests
  • Performance tests
  • All with ASAN
  • All with TSAN
  • All with MSAN
  • All with UBSAN
  • All with Coverage
  • All with Aarch64
  • All Regression
  • Disable CI Cache

Regression jobs to run:

  • Fast suites (mostly <1h)
  • Aggregate Functions (2h)
  • Alter (1.5h)
  • Benchmark (30m)
  • ClickHouse Keeper (1h)
  • Iceberg (2h)
  • LDAP (1h)
  • Parquet (1.5h)
  • RBAC (1.5h)
  • SSL Server (1h)
  • S3 (2h)
  • Tiered Storage (2h)

SmitaRKulkarni and others added 2 commits March 19, 2026 12:42
…rt-94467-Improve_catalog_show_tables

Revert "Revert "Improve catalog show tables query""
The original test leaks the failpoint and all the tests in this module
after this test can hit a 10-second sleep on `tryGetTableImpl`. It
doesn't happen in practice, because this is the last test in the module.

However, for the sake of keeping CI stable we'll fix that leak,
considering the fix is trivial.
@github-actions
Copy link

Workflow [PR], commit [94cfd2a]

@mkmkme
Copy link
Collaborator Author

mkmkme commented Mar 19, 2026

I also ran an audit check and let Claude analyze the output. Here's the result:

Concern Valid? Upstream too? Worth fixing?
1. Unbounded block size Yes Yes Nice-to-have, low practical impact
2. require_metadata_access bypassed Technically yes Yes Arguable — current behavior is arguably correct
3. Failpoint leak Yes Yes Yes, trivial fix

I have fixed the last one.

Copy link
Collaborator

@arthurpassos arthurpassos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If my eyes are sharp, this seems to be a 1:1 match. LGTM

@ianton-ru
Copy link

Just to history

Audit Report: PR #1552 (Altinity/ClickHouse) - Improve catalog show tables query

PR: Antalya 26.1 Backport of #97062 - Improve catalog show tables query #1552
Commits in scope: 3a441ec3115 ("Merge pull request ClickHouse#97062 from ClickHouse/revert-96826-revert-94467-Improve_catalog_show_tables"), 94cfd2a0808 ("test_database_glue: fix the failpoint leak")


AI audit note: This review comment was generated by AI.

Confirmed defects

Low: name-only system.tables path no longer honors max_block_size

Impact: A single datalake database with many visible tables can be emitted as one oversized chunk for SHOW TABLES-style queries, causing unnecessary memory spikes and defeating block-size based pacing.
Anchor: src/Storages/System/StorageSystemTables.cpp / fillTableNamesOnly(), generate()
Trigger: Run SHOW TABLES FROM <datalake_db> or SELECT name[, database] FROM system.tables ... when one database has more matching tables than max_block_size.
Why defect: The normal iterator path stops at rows_count < max_block_size, but the new fast path appends every matching table in the database before returning, then adds the whole count back to rows_count.

Affected subsystem and blast radius: name-only system.tables and rewritten SHOW TABLES queries over datalake catalogs.

Code evidence:

size_t rows_added = fillTableNamesOnly(res_columns);
rows_count += rows_added;
continue;
for (const auto & table_detail: table_details)
{
    ...
    res_columns[res_index++]->insert(table_detail.name);
    ++count;
}

Fix direction (short): cap the fast path at remaining block capacity and keep per-database iteration state across generate() calls.
Regression test direction (short): create more than max_block_size datalake tables and assert system.tables/SHOW TABLES produces bounded chunks instead of one oversized block.

Low: database_datalake_require_metadata_access=1 is bypassed for name-only table listing

Impact: In strict mode, SHOW TABLES and equivalent name-only system.tables queries can now succeed and reveal table names even when fetching table metadata would fail; operators lose the documented fail-on-metadata-inaccessibility behavior for those queries.
Anchor: src/Storages/System/StorageSystemTables.cpp / name-only fast path, src/Databases/DataLake/DatabaseDataLake.cpp / getLightweightTablesIterator(), src/Core/Settings.cpp / database_datalake_require_metadata_access
Trigger: A datalake catalog lists a table, metadata access for that table fails, database_datalake_require_metadata_access=1, and the query only requests table names.
Why defect: The old lightweight path still called tryGetTableImpl(..., true, ...) and enforced the setting on metadata-fetch failures. The new implementation returns names directly from catalog->getTables() and never performs metadata access, so the strict setting cannot fire.

Affected subsystem and blast radius: strict metadata-access behavior for datalake-backed SHOW TABLES and name-only system.tables reads.

Code evidence:

iceberg_tables = catalog->getTables();
...
result.emplace_back(table_name);
DECLARE(Bool, database_datalake_require_metadata_access, true, R"(
Either to throw error or not if we don't have rights to get table's metadata ...
)", 0)

Fix direction (short): either preserve strict-mode checks for name-only listing when the setting is enabled, or explicitly redefine/document the setting so this becomes intentional.
Regression test direction (short): inject metadata-access failure and assert SHOW TABLES FROM <catalog> still errors when database_datalake_require_metadata_access=1.

Coverage summary

Scope reviewed: Full call graph for the PR's changed paths: InterpreterShowTablesQuery rewrite into system.tables, StorageSystemTables filtering/generation fast path, IDatabase lightweight iterator contract, DatabaseDataLake full and lightweight iterator behavior, and the failpoint/test fix in test_database_glue.
Categories failed: Block-size enforcement; strict metadata-access setting contract.
Categories passed: Caller migration back to full iterators where StoragePtr is required (system.columns, system.completions, system.iceberg_history, SYSTEM DROP REPLICA); datalake error propagation in full iterator path; failpoint leak fix; reviewed C++ lifetime/race/deadlock/exception-safety concerns in touched code showed no confirmed defects.
Assumptions/limits: Static audit only; no runtime reproduction or CI execution performed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants