Skip to content

Read Iceberg catalog once during cluster requests. - #2729

Open
ianton-ru wants to merge 4 commits into
antalya-26.8from
bugfix/antalya-26.8/object-storage-cluster-catalog-once
Open

ianton-ru wants to merge 4 commits into
antalya-26.8from
bugfix/antalya-26.8/object-storage-cluster-catalog-once

Conversation

@ianton-ru

Copy link
Copy Markdown

Changelog category (leave one):

  • Bug Fix (user-visible misbehavior in an official stable release)

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

Read Iceberg catalog once during cluster requests.

Documentation entry for user-facing changes

Solved #2721

CI/CD Options

Exclude tests:

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

Regression jobs to run:

  • Fast suites (mostly <1h)
  • Aggregate Functions (2h)
  • Alter (1.5h)
  • Benchmark (30m)
  • CAS (content-addressed storage; Antalya only)
  • ClickHouse Keeper (1h)
  • Iceberg (2h)
  • LDAP (1h)
  • OAuth (5m)
  • Parquet (1.5h)
  • RBAC (1.5h)
  • SSL Server (1h)
  • S3 (2h)
  • S3 Export (2h)
  • Swarms (30m)
  • Tiered Storage (2h)

ianton-ru and others added 2 commits October 28, 2026 15:34
Re-analyzing the original AST resolved a `DataLakeCatalog` table again and repeated the REST catalog lookup.

Co-authored-by: Cursor <cursoragent@cursor.com>
Keep existing header analysis for all other clustered storage reads.

Co-authored-by: Cursor <cursoragent@cursor.com>
@github-actions

github-actions Bot commented Oct 28, 2026 •

Copy link
Copy Markdown

Workflow [PR], commit [6a9a4cb]

@ianton-ru

Copy link
Copy Markdown
Author

@codex review

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 28, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-29T08:47:36.449232Z dbe0f46 Manual request
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Something went wrong. Try again later by commenting “@codex review”.

Failed to set up container
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@ianton-ru

Copy link
Copy Markdown
Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: dbe0f46f39

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/Storages/IStorageCluster.cpp Outdated
sample_block = InterpreterSelectQueryAnalyzer::getSampleBlock(query_to_send, context, SelectQueryOptions(processed_stage));
/// For an unchanged query, calculate the header from the resolved query tree. Re-analyzing
/// the original AST resolves DataLakeCatalog tables once more and repeats a remote catalog lookup.
if (canReuseResolvedQueryTreeForHeader() && query_to_send == query_info.query && query_info.query_tree)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Reuse the resolved tree after query rewrites

When a DataLakeCatalog query contains a JOIN or a table-referencing IN/WHERE that must remain on the initiator, updateQueryWithJoinToSendIfNeeded replaces query_to_send with a new AST. This condition then falls through to InterpreterSelectQueryAnalyzer::getSampleBlock, which resolves the catalog table again, so these supported cluster requests still perform duplicate catalog metadata reads despite the stated fix. Preserve the rewritten query tree for header calculation, or otherwise calculate the rewritten header without re-resolving the catalog; a focused test with a catalog table joined to a local Memory table would expose the duplicate lookup.

Useful? React with 👍 / 👎.

Re-analyzing the rewritten shard AST resolved the catalog table a second time.

Co-authored-by: Cursor <cursoragent@cursor.com>
@ianton-ru

Copy link
Copy Markdown
Author

@blau-ai

@blau-ai

blau-ai commented Oct 29, 2026

Copy link
Copy Markdown
Collaborator

CI triage for f281aab (run 40862005818, still in progress)

TL;DR: 6 red checks. 1 is caused by this PR (Swarms regression, swarm joins / swarm join sanity / swarm union). The other 5 (4 integration jobs and OAuth) fail the same way on the sibling PRs #2715 and #2730, so they come from the base branch. The PR's own new tests pass.

Check Verdict
RegressionTestsRelease / Swarms ❌ PR-caused (joins/union with a DataLakeCatalog table)
Integration (asan, db disk, old analyzer, 5/8): 21× test_export_partition_iceberg pre-existing
Integration (asan, db disk, old analyzer, 6/8): 17× test_database_backup pre-existing
Integration (asan, db disk, old analyzer, 7/8): 2× test_custom_s3_disk_remote_url_allow_hosts pre-existing
Integration (asan, targeted): 18 failures, all from the two suites above pre-existing
RegressionTestsRelease / OAuth (/oauth/access control, /oauth/groups) pre-existing

❌ Swarms regression (caused by this PR)

The base-branch failures appear on #2730 and #2715 too: cluster discovery/check cluster discovery with wrong cluster name and node failure/cpu overload*. Those two runs have 15 failed rows each. This PR has 489. The extra ones are swarm joins/join clause, swarm join sanity/{join subquery, join with clause} and swarm union/union clause, plus node failure/check restart swarm node (see below).

Nearly all the new failures have the same error:

DB::Exception: Not found column __table2.boolean_col in block. There are only columns: __table1.boolean_col, __table1.long_col, ...
: While executing Remote. (NOT_FOUND_COLUMN_IN_BLOCK)

Counts: __table2 ×143, __table5 ×48, __table3 ×38, __table4 ×7. In every failing scenario the catalog table (database_….namespace_….table_…) is not the left table of the join. Examples: merge_tree_table … with database_….table_… in allow mode … LEFT OUTER JOIN, iceberg(...) with database_….table_… in local mode … ASOF JOIN.

Root cause: this is the new branch at src/Storages/IStorageCluster.cpp:739:

else if (canReuseResolvedQueryTreeForHeader() && query_to_send == query_info.query && query_info.query_tree)
    sample_block = getHeaderForProcessingStage(column_names, storage_snapshot, query_info, context, processed_stage);

When the join is not rewritten (allow mode, or local mode when needsInitiatorLocalJoin() is false), query_to_send is still the full query with the JOIN. For a query with a JOIN, getHeaderForProcessingStage builds the header from extractLeftTableExpression(join_tree) (src/Interpreters/getHeaderForProcessingStage.cpp:167). That is always the left table. If the catalog table is the right side (or deeper in the join tree or a subquery), the header holds the left table's __table1.* columns while the plan expects __table2.* / __tableN.*. Before this PR the header came from re-analyzing query_to_send, so this case never hit that code.

Suggested fix: use the shortcut only when it is safe, i.e. when this storage is the left/only table expression. For example:

else if (canReuseResolvedQueryTreeForHeader() && query_to_send == query_info.query && query_info.query_tree
         && query_info.table_expression
         && extractLeftTableExpression(query_info.query_tree->as<QueryNode &>().getJoinTree()) == query_info.table_expression)

The simpler option is to skip the shortcut whenever the query has a JOIN (!hasJoin(...)), which gives up the optimization for joins in allow mode. Then add an integration case to test_database_iceberg where the catalog table is on the right of a join with object_storage_cluster_join_mode='allow', and one with local + ASOF. The two new tests only cover the catalog table on the left (or the rewritten-join path), which is why they passed.

node failure/check restart swarm node (AssertionError, together with ALL_CONNECTION_TRIES_FAILED / SOCKET_TIMEOUT errors in the report) passes on both sibling PRs. I can't tell yet whether it's related. Re-check it after the fix.

Report: swarms/report.html

Pre-existing (not caused by this PR)

The evidence is the same for all of these: the same test sets with the same counts fail on #2730 (fa0e5fe) and #2715 (afa617e), which don't touch this code.

  • 5/8, test_storage_iceberg_with_spark/test_export_partition_iceberg.py (21 tests): fails in setup, before reaching any cluster code:
    Not enough privileges. To execute this query, it's necessary to have the grant SHOW NAMED COLLECTIONS SECRETS ON s3. (ACCESS_DENIED) on CREATE TABLE … ENGINE=IcebergS3(s3, …).
  • 6/8, test_database_backup/test.py (17 tests): Code: 210. DB::NetException: Connection refused (172.16.1.8:9000). The server doesn't come back up.
  • 7/8, test_backward_compatibility/test_custom_s3_disk_remote_url_allow_hosts.py (2 tests): Timed out while waiting for instance 'node' … to start in started_cluster.
  • Integration targeted: this job only re-ran the 19 failing test_database_backup / test_backward_compatibility tests above, and they failed again the same way. It did not include test_database_iceberg.
  • OAuth regression: /oauth/access control and /oauth/groups fail with AssertionError: Oops! Assertion failed, identically (7 failed rows) on Fix early manifest deletion in CAS GC #2730 and Fixes for Iceberg Puffin Delete Vectors #2715.

PR's own tests ✅

test_database_iceberg ran in shard 4/8: 104 passed, 2 skipped, 0 failed. That includes test_cluster_select, test_object_storage_cluster_loads_rest_catalog_table_once and test_object_storage_cluster_join_loads_rest_catalog_table_once.

Analysis is static, from the praktika/regression reports. A fix would be validated by the next CI run on this PR.

The resolved tree kept the outer `__tableN` alias, so a non-leftmost catalog table did not match the shard block.

Co-authored-by: Cursor <cursoragent@cursor.com>
@ianton-ru

Copy link
Copy Markdown
Author

@blau-ai

@blau-ai

blau-ai commented Oct 30, 2026

Copy link
Copy Markdown
Collaborator

CI triage for 6a9a4cb

Verdict: 6 failing checks. 0 are clearly caused by this PR and 5 already fail on the base branch. One swarms scenario might be linked to this PR and needs a re-run to tell.

Check Failing tests Verdict
Integration (amd_asan_ubsan, targeted) test_database_backup ×16, test_backward_compatibility/test_custom_s3_disk_remote_url_allow_hosts ×2 Pre-existing
Integration (amd_asan_ubsan, db disk, old analyzer, 5/8) test_storage_iceberg_with_spark/test_export_partition_iceberg.py ×21 Pre-existing
Integration (amd_asan_ubsan, db disk, old analyzer, 6/8) test_database_backup ×17 Pre-existing
Integration (amd_asan_ubsan, db disk, old analyzer, 7/8) test_custom_s3_disk_remote_url_allow_hosts ×2 Pre-existing
Regression release oauth 7 scenarios in /oauth/groups + /oauth/access control Pre-existing
Regression release swarms 3 known scenarios + node failure/check restart swarm node Known ones pre-existing; the restart scenario is unclear and touches this PR's code path (details below)

How I checked "pre-existing": I compared against praktika result_pr.json and the regression job logs from unrelated antalya-26.8 PRs #2735, #2737, #2730 (and #2715 for swarms). They show the same test files failing in the same shards.

Integration tests: pre-existing, not this PR

None of these tests use IStorageCluster / StorageObjectStorageCluster or a DataLakeCatalog database.

Regression oauth: pre-existing

The same 7 scenarios fail on #2735 and #2730, e.g. /oauth/groups/roles filter limits groups and /oauth/access control/missing default profile does not grant unrestricted access.

Regression swarms: mostly pre-existing, one scenario needs a look

These 3 fail on every sibling PR (#2735, #2737, #2730, #2715):

  • cluster discovery/check cluster discovery with wrong cluster name
  • node failure/cpu overload
  • node failure/cpu overload all swarm nodes

node failure/check restart swarm node fails only here. It passed on the 4 sibling runs I checked.

Expected exit code in [138], but got: 0
  File 'swarms/tests/node_failure.py', line 108 in 'run_long_query'

What the test does (node_failure.py):

  • It runs SELECT count(), hostName() FROM datalakecatalog_db_….<table> WHERE NOT ignore(sleepEachRow(1)) … SETTINGS object_storage_cluster='static_swarm_cluster'.
  • At the same moment it stops and restarts a random swarm container (restart_random_swarm_node(delay=0.0)).
  • It expects the query to be cancelled (138). Here the query completed successfully.

This goes through exactly the branch the PR adds: a DataLakeCatalog table, analyzer on, canReuseResolvedQueryTreeForHeader() == true. On the initiator, the header now comes from the already-resolved tree instead of re-analysing the AST, which skips one REST catalog round-trip. Shard dispatch therefore happens earlier relative to the container restart.

The test is timing-sensitive by design: its own comments call stopping a node mid-query "inherently racy". So the likely explanation is that the shorter dispatch path changed the race, not that results are wrong. But this is one sample and I can't rule out a behaviour change from one run.

Suggested next step:

  1. Re-run RegressionTestsRelease / Swarms (and ideally aarch64 swarms) on this PR.
  2. If check restart swarm node passes, it was the race.
  3. If it keeps returning 0, check whether the initiator now skips or retries the restarted replica. For example, look at system.query_log / the initiator server log in the swarms artifacts for the Remote connections to the restarted node. In that case the test's expectation (or the PR) needs adjusting.

Other notes

  • Because of the failures above, the arm_binary distributed plan, amd_tsan and amd_msan integration shards were skipped. They give no signal on this PR yet.
  • The PR's 3 new test_database_iceberg tests (test_object_storage_cluster_*loads_rest_catalog_table_once) are not in any failure list. result_pr.json only lists non-OK tests, so I can't positively show them as OK.
  • No fixes suggested for the pre-existing failures: they belong on the base branch (backup multipart crash, missing SHOW NAMED COLLECTIONS SECRETS grant in the export-partition test, custom_s3_disk BWC startup, oauth, swarms).

This branch has not been deployed

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

Labels

antalya antalya-26.8 Session label (releasy session config) port-antalya PRs to be ported to all new Antalya releases

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants