Workflow [PR], commit [71da382]

Summary: ✅

AI Review

Summary

This PR is now test-only: it adds test_writes_detach_attach and test_writes_restart to verify that IcebergS3, IcebergAzure, and IcebergLocal tables remain writable after explicit DETACH/ATTACH and after instance.restart_clickhouse, for Iceberg format versions 1 and 2. The previous shared object-storage source change was removed, the remaining bot-authored thread is resolved correctly, and I found no current correctness or stability issues in the added coverage.

Final Verdict

Status: ✅ Approve

Dear @alexey-milovidov, @divanik, you haven't been active on this PR for 30 days. You will be unassigned. Will you continue working on it? If so, please feel free to reassign yourself.

Picked this up to advance it. Summary of what I did and an important finding for you to decide on.

Done

• Merged current master into the branch (dbab77d6b3d..23c16826505, 0 conflicts). This picks up the fix for the FunctionsStress unit-test flake.
• Resolved the three addressed review threads from @divanik (comment wording, table name, and the real-server-restart test — test_writes_restart now calls instance.restart_clickhouse()).
• Replied to the clickhouse-gh thread about Azure (left open for a maintainer call — see below).

Key finding: the source change is now redundant

The actual fix for #85413 already landed on master in 7ee8dd3c39c ("Keep IcebergLocal writable after DETACH/ATTACH or server restart"). It fixes the real cause at the configuration layer: StorageLocalConfiguration::createObjectStorage now ignores is_readonly and always builds a writable LocalObjectStorage (previously readonly=true on ATTACH permanently blocked writes via throwIfReadonly).

Because of that, this PR's change to the shared registerStorageObjectStorage.cpp helper (passing is_readonly=false for ATTACH) is:

• a no-op for Local (it ignores the flag) — i.e. no effect on the stated local-Iceberg goal,
• a no-op for S3/HDFS (they also ignore the flag),
• the only behavioral effect is on Azure, where explicit ATTACH would now do a container existence check (and create the container only if it truly doesn't exist) — exactly the concern clickhouse-gh raised.

This also explains the Bugfix validation (integration tests) failure: all 36 tests pass on a plain master binary (the bug is already fixed), so "Failed to reproduce the bug" is structural and cannot be made green by any code change here.

The genuinely valuable part of this PR is the added integration coverage (test_writes_detach_attach, test_writes_restart) across s3/azure/local + restart.

Suggestion: consider reducing this to a test-only PR (drop the registerStorageObjectStorage.cpp change, which avoids the Azure concern, and move the Changelog category off "Bug Fix"), or close it as covered by 7ee8dd3c39c. I did not change the approach unilaterally since @antonio2368 already approved — your call.

Unrelated CI failures (not caused by this PR)

• Unit tests (msan, function_prop_fuzzer) → FunctionsStress — known flake, tracked in FunctionsStress.stress #104877; fix is in the master I just merged.
• Stateless tests (amd_tsan, parallel, 1/2) → 00096_obfuscator_save_load, 00175_obfuscator_schema_inference — obfuscator tests, unrelated to iceberg. @groeneai, please investigate https://github.com/ClickHouse/ClickHouse/actions/runs/26867560751/job/79250997949 and provide a fix in a separate PR if it isn't already a tracked flake.
• Unit tests (asan_ubsan) already shows 0 failures on re-check.

@alexey-milovidov investigated. The obfuscator amd_tsan failures (00096_obfuscator_save_load, 00175_obfuscator_schema_inference) were caused by #104690 (Add UntrackedMemory asynchronous metric, merged 2026-06-02 21:01 UTC), already reverted by #106365 (merged 2026-06-03 06:28 UTC). No separate fix PR is needed; this PR just needs a master merge to pick up the revert.

CIDB evidence:

• Daily failure counts (master + PR): 1-3/day → spike 18 on 06-02 (after merge) → 207 on 06-03 (102 each test) → 1 on 06-04 (after revert).
• Master-only post-revert (since 2026-06-03 06:30 UTC): 32+ master tips ran Stateless tests (amd_tsan, parallel, 1/2) for both tests with 0 failures. Both deterministic (10/10 without randomization) and flaky shapes were observed pre-revert.
• All 9 master failures and ~150 PR failures occurred on commits that do NOT contain 3930f8abb8ac (the revert) - confirmed via git merge-base --is-ancestor 3930f8abb8ac <sha>. PR Add integration tests for attach/detach and restart of Iceberg tables #91583's failing commit dbab77d6b3da is pre-revert, as is PR Fix bucket-aware merging dropped by analyzer with ARRAY JOIN over remote source #104505's bbb37375... (this one's only 06-04 sighting).
• A reapply (Reapply "Add UntrackedMemory asynchronous metric" #106386 by mstetsyuk) is open; presumably the obfuscator save path will need a fix before that lands.

Tracking this in our chronic-flaky umbrella tasks for 00096_obfuscator_save_load and 00175_obfuscator_schema_inference so any post-reapply recurrence is caught immediately.

Picked this up via /continue-pr:

• Merged master (the branch was ~395 commits behind and red); no conflicts.
• Reduced the PR to test-only. The local-Iceberg writability fix for Local Iceberg table not insertable after server restart #85413 already landed on master in 7ee8dd3c39c at the configuration layer (StorageLocalConfiguration::createObjectStorage ignores is_readonly; StorageS3Configuration and HDFS already do). The registerStorageObjectStorage.cpp change here was therefore redundant for the stated goal and would have introduced the Azure-only behavioral change flagged in review (an extra existence check / container-create path in AzureBlobStorage::getContainerClient on explicit ATTACH). That file now matches master; only the new integration tests (test_writes_detach_attach, test_writes_restart) remain.
• Updated the changelog category to Not for changelog (the label is already pr-not-for-changelog) and refreshed the title/body, since the bug is already fixed on master and this PR is now pure regression coverage.
• Resolved the outstanding review thread accordingly.

The previously-red CI (Bugfix validation, Stress test (amd_tsan) hung-check) was against the stale pre-merge SHA; CI will re-run on 7371cc6ee36. With no source change, those checks cannot be caused by this PR.

@groeneai, investigate the failure: 04295_04278_materialized_cte_in_union_runtime_set in the Stateless tests (amd_llvm_coverage, ParallelReplicas, s3 storage, parallel) report and provide a fix in a separate PR. If the fix is already in progress, link it here.

This is unrelated to this PR (which is test-only, adding two Iceberg integration tests). The test is failing on master itself and across many unrelated PRs since 2026-06-06 (e.g. master runs on 2026-06-08, and PRs #106692, #84041, #106657, #100177, #104625, #106178). The failure is UNKNOWN_TABLE while executing Remote under ParallelReplicas, even though the relevant planner fix 9335a9cefe1 ("Drop all DelayedMaterializingCTEStep's in the Set plan") is already present, suggesting a remaining ParallelReplicas-specific gap or flakiness.

@alexey-milovidov, fix in flight: #106717.

Stop-gap pin of enable_parallel_replicas = 0 at the top of the test. CIDB confirms 12 hits over 14 days, all on Stateless tests (amd_llvm_coverage, ParallelReplicas, s3 storage, parallel); master red today on commits 38129a8480db and bf043eba3728; 11 unrelated PRs affected (incl. this one).

Mechanism: under enable_parallel_replicas = 1, the temporary materialized CTE storage _materialized_cte_ct_<hash> is registered on the initiator only; the follower replica running the Remote step raises UNKNOWN_TABLE: Unknown table expression identifier '_materialized_cte_ct_<hash>' in QueryAnalyzer::initializeQueryJoiningTreeWithTableExpressions. The planner-side fix 9335a9cefe1 (removeAllDelayedMaterializingCTEsStep) is already on master but only covers single-server execution; the parallel-replicas distributed-query path is uncovered.

The proper fix - making materialized CTEs work under parallel replicas - lives in MaterializingCTEStep plus the ParallelReplicas distributed-query construction path. That's @ novikd's MaterializedCTE area; tracked separately so this PR can unblock master immediately.

Session id: cron:clickhouse-ci-task-worker:20260608-120900

@alexey-milovidov — closing the loop on this directive.

The flaky 04295_04278_materialized_cte_in_union_runtime_set is now resolved by your own revert PR #106795 (merged 2026-06-09T02:30:18Z), which removed the entire materialized-CTE feature added by #105041 — including the failing test file.

Stop-gap fix PR #106717 (pinned enable_parallel_replicas = 0) is therefore obsolete and has been closed: #106717 (comment)

If/when materialized CTEs are reintroduced, the proper parallel-replicas support (initiator-side _materialized_cte_ct_<hash> registration is currently invisible to follower replicas executing the Remote step) should be folded into the new implementation rather than re-pinning the test setting.

Session: cron:clickhouse-ci-task-worker:20260609-071900

Merged master into the branch to refresh the ~3-day-old base and pick up the fix for the only red.

The sole CI failure was Tests/04295_04278_materialized_cte_in_union_runtime_set in the Stateless tests (amd_llvm_coverage, ParallelReplicas, s3 storage, parallel) job — unrelated to this Iceberg test-only PR. That test was broadly flaky on master under parallel replicas (19 failures on 2026-06-08 per CIDB) and was tagged no-parallel-replicas on master in b95136881bc, which this merge now brings into the branch, so it will be skipped under ParallelReplicas.

No conflicts; the PR diff is unchanged (the two new test_writes_detach_attach / test_writes_restart integration tests). No unresolved review threads; approved by @antonio2368.

LLVM Coverage Report

Changed lines: No C/C++ source files changed — skipping uncovered code analysis.

Newly covered by added/modified tests: 879 line(s), 54 function(s) across 132 file(s) · Details

Full report