Fixed iceberg alter table modify rename

[subkanthi]

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):

Backport of #1594, Fixes Alter table ADD/DROP/RENAME/MODIFY column for Iceberg tables.

Documentation entry for user-facing changes

Fixes Alter table ADD/DROP/RENAME/MODIFY column for Iceberg tables.

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)
• S3 Export (2h)
• Swarms (30m)
• Tiered Storage (2h)

[github-actions]

Workflow [PR], commit [889ec3f]

[subkanthi]

continuation of #1794

[subkanthi]

Audit Review: PR #1841 — Fixed iceberg alter table modify rename

AI audit note: This review comment was generated by AI (claude-4.6-opus).

PR: #1841 — Iceberg ALTER TABLE schema-change via REST catalog

---

Call Graph

StorageObjectStorage::alter
  → configuration->update(object_storage, context)
  → params.apply(new_metadata, context)
  → configuration->alter(params, context, storageID, catalog)
     → DataLakeConfiguration::alter
        → assertInitializedDL()
        → IcebergMetadata::alter(params, shared_from_this(), context, storage_id, catalog)
           → Iceberg::alter(params, context, object_storage, data_lake_settings, ...)
              → MetadataGenerator::generate{Add,Drop,Modify,Rename}ColumnMetadata
              → writeMetadataFileAndVersionHint
              → catalog->updateMetadata(ns, table, path, metadata_json)
                 → buildUpdateMetadataRequestBody(ns, table, new_snapshot)
                 → sendRequest(endpoint, request_body)
  → database->alterTable (skipped for datalake catalogs)
  → setInMemoryMetadata(new_metadata)

Key Invariants

1. Schema ID in the REST request must match the old schema ID that the catalog currently holds (assert-current-schema-id requirement).
2. On catalog commit failure, orphan metadata files in object storage must be cleaned up.
3. data_lake_settings must not be mutated unsafely across threads.
4. The retry loop must converge and must not adopt stale/wrong metadata.
5. checkAlterIsPossible must be called before alter to validate commands.

---

Confirmed Defects

Medium: old_schema_id = new_schema_id - 1 assumption is fragile and can send wrong REST requirement

• Impact: If an Iceberg table has non-contiguous schema IDs (e.g. schema-id jumps from 0 to 5 due to external schema evolutions), the assert-current-schema-id requirement sent to the REST catalog will be wrong. The REST catalog will reject the commit with a requirement-violation error, making ALTER TABLE permanently fail for such tables even though the operation is valid.
• Anchor: src/Databases/DataLake/RestCatalog.cpp / buildUpdateMetadataRequestBody — const Int32 old_schema_id = new_schema_id - 1;
• Trigger: Any Iceberg table whose current-schema-id in the metadata JSON was not produced by this code's MetadataGenerator (e.g. table created/evolved by Spark with ID gaps).
• Why defect: The MetadataGenerator always increments current-schema-id by 1, so new_schema_id - 1 happens to equal the current value in the metadata. However, buildUpdateMetadataRequestBody receives the entire metadata JSON blob and could read the actual old schema ID directly from the schemas array or from the metadata's current-schema-id before mutation. Deriving it arithmetically is brittle and relies on an undocumented invariant of MetadataGenerator. If MetadataGenerator ever changes its numbering (or metadata is loaded from a table with gaps), this breaks silently.
• Fix direction: Pass the original current-schema-id value (before increment) as an explicit parameter, or read it from the metadata blob's pre-increment state rather than computing new - 1.
• Regression test direction: Unit test with current-schema-id=5 and new_schema_id=6; verify requirement says current-schema-id=5, not computed from 6 - 1 (currently happens to pass but test should lock the contract).

Medium: const_cast on data_lake_settings creates mutable alias to const reference — unsynchronized cross-thread mutation risk

• Impact: IcebergMetadata stores DataLakeStorageSettings & obtained via const_cast from configuration_->getDataLakeSettings() (which returns const &). The alter function then mutates data_lake_settings[iceberg_metadata_file_path] through this reference. Since DataLakeStorageSettings is also read by the background metadata prefetch task and by concurrent SELECT queries (both reading iceberg_metadata_file_path), this is an unsynchronized write to shared state. In ASan/TSan builds this is a data race (UB); in release builds it could produce torn reads.
• Anchor: src/Storages/ObjectStorage/DataLakes/Iceberg/IcebergMetadata.cpp line 267: const_cast<DataLakeStorageSettings &>(configuration_->getDataLakeSettings()) and src/Storages/ObjectStorage/DataLakes/Iceberg/Mutations.cpp lines 830-831: data_lake_settings[...] = metadata_name;
• Trigger: ALTER TABLE ... DROP/ADD/MODIFY COLUMN concurrent with any read query or background prefetch that accesses iceberg_metadata_file_path.
• Why defect: const_cast to strip const from a reference returned by an API that intends it to be const is UB if the original object was declared const. Even if it is not declared const, the mutation is unsynchronized.
• Fix direction: Either pass the new metadata path back via a return value / output parameter, or use a proper synchronized setter on the configuration object.
• Regression test direction: TSan integration test running ALTER TABLE concurrently with SELECT queries.

Medium: checkAlterIsPossible silently skips validation when current_metadata is null

• Impact: Before this PR, checkAlterIsPossible called assertInitializedDL() which would throw LOGICAL_ERROR if metadata was not initialized. Now it silently returns without validating the alter commands. This means an ALTER on an uninitialized table (e.g. due to a metadata load failure) will pass validation and proceed to alter which will throw a different, less clear error from assertInitializedDL() inside alter.
• Anchor: src/Storages/ObjectStorage/DataLakes/DataLakeConfiguration.h lines 177-181.
• Trigger: ALTER TABLE on a datalake table where metadata failed to load (e.g. transient S3 error during table open).
• Why defect: The old code failed fast with a clear error. The new code allows the operation to proceed further before failing, potentially with a confusing error or partial side effects (e.g. params.apply(new_metadata, context) in StorageObjectStorage::alter runs before configuration->alter is called).
• Fix direction: Keep the assertInitializedDL() check, or at minimum throw explicitly when current_metadata is null rather than silently skipping.
• Regression test direction: Integration test that drops metadata cache, then tries ALTER TABLE and expects a clear error message.

Low: Catalog commit failure with !wrote_ok && file_exists silently adopts a file that might be from a different/stale operation

• Impact: When writeMetadataFileAndVersionHint fails but the file already exists in object storage (written by a concurrent operation or a previous failed attempt), the code proceeds to call catalog->updateMetadata. If the catalog update also fails, it calls continue (retry). If it succeeds, it adopts the file. The adopted file may have been written by a different schema evolution, or contain stale metadata from a previous failed attempt that was not cleaned up.
• Anchor: src/Storages/ObjectStorage/DataLakes/Iceberg/Mutations.cpp lines 790-828.
• Trigger: Two concurrent ALTER TABLE operations on the same Iceberg table, both targeting the same metadata version. One writes the file, the other's write fails, but it finds the file and adopts it.
• Why defect: The content of the existing file is not verified. The code assumes that if a file with the expected name exists, it has the right content. This is plausible for version-numbered metadata files but is not guaranteed, especially if object storage has eventual consistency or if the file was written with different schema changes.
• Fix direction: After finding an existing file, read it back and verify the current-schema-id and schema content match what this operation expects before adopting it.
• Regression test direction: Test with two concurrent schema changes targeting the same version; verify only one succeeds.

Low: removeObjectIfExists in orphan cleanup path swallows all exceptions silently

• Impact: If the metadata file cleanup fails (e.g. permission error, network timeout), the only evidence is a warning log. The orphan metadata file remains in object storage. While the code then throws DATALAKE_DATABASE_ERROR, the orphan file is never cleaned up and accumulates over time.
• Anchor: src/Storages/ObjectStorage/DataLakes/Iceberg/Mutations.cpp lines 811-818.
• Trigger: Object storage permission error or transient failure during cleanup.
• Why defect: This is an acceptable trade-off (the alternative is losing the catalog-failure error), but the swallowed exception means orphan files silently accumulate. The test test_alter_orphan_metadata_cleanup_on_catalog_failure only tests the happy path of successful cleanup.
• Fix direction: Consider adding a background cleanup mechanism for orphan metadata files, or document this as a known limitation.
• Regression test direction: Test where both catalog commit and file cleanup fail; verify the error message still mentions catalog failure.

---

Coverage Summary

• Scope reviewed: All 16 changed files (776 insertions, 87 deletions). Full call graph from StorageObjectStorage::alter through Iceberg::alter to RestCatalog::updateMetadata and buildUpdateMetadataRequestBody.
• Categories passed: Input validation/error paths, REST request body construction (both schema and snapshot paths), retry loop convergence (bounded by MAX_TRANSACTION_RETRIES=100), memory lifetime (Poco JSON ref-counted Ptr), exception safety/partial rollback (catalog failure rolls back metadata file).
• Categories failed: Schema ID derivation correctness (Medium), thread safety via const_cast (Medium), checkAlterIsPossible validation bypass (Medium), unverified file adoption (Low), orphan cleanup resilience (Low).
• Not applicable: Iterator invalidation, integer overflow (Int32 schema IDs incremented by 1), RAII violations.
• Assumptions/limits: Static analysis only. The old_schema_id = new - 1 computation is currently correct in practice because MetadataGenerator always increments by 1; the defect is about brittleness and missing contract enforcement.

[alsugiliazova]

It makes sense to insert value bigger than max Int32 here

[subkanthi]

updated

[alsugiliazova]

Verification Report: PR #1841 — Iceberg ALTER TABLE Fix

PR: Altinity/ClickHouse#1841
Commit: 5be2aba12d1c209afce1f876ac3146d0c53bac62 → antalya-26.3
Reviewed: 2026-06-01
CI run: 26732741417 · report

Change: Backport of #1594 — fix ALTER TABLE ADD/DROP/RENAME/MODIFY COLUMN for Iceberg tables. Touches RestCatalog, IcebergMetadata, Mutations.cpp, integration tests.

---

Verdict

CI status	RED (18 fail/error jobs)
Block merge on CI?	Yes — one PR-scoped integration failure
Core fix validated?	Partially — iceberg regression green (both suites, both arches); integration test_alter_drop_column_without_reload red on all shards

---

PR-caused failure (fix before merge)

Test	Shards	Evidence
test_database_iceberg/test.py::test_alter_drop_column_without_reload	amd_asan 5/6, amd_binary 1/5, arm_binary 2/4 (+ 10 targeted reruns)	12 fails / 0 OK in Altinity CI (90d), only on PR builds — directly in PR scope (DROP COLUMN without reload)

This is the only failure clearly tied to the PR's iceberg ALTER work.

---

Not PR regressions

Failure	Why ignore for #1841
1124 regression "new fails" (swarms)	Combinatorial diff — swarms fail rate ~37.5% on PR build, same pattern as other PRs; not a rate increase vs typical PR noise
Settings regression (export_merge_tree_partition_all_on_error)	Missing snapshot — not in PR diff; base-branch drift
Version, disk_level_encryption, tiered_storage, kafka, s3_gcs_2	Unrelated suites; kafka errored in 71ms (infra)
SQLLogic test	Timeout — infra
test_remote_initiator_after_non_remote[s3] (amd_asan 4/6)	Flaky; fix tracked in PR #1856
Targeted reruns (test_parallel_quorum_actually_parallel[*])	Unrelated; flaky quorum test
03443_shared_storage_snapshots (+ backup/scraping on same shard)	Unrelated stateless; likely flake/infra on arm_asan sequential

---

What passed

All builds · fast test · unit tests · most stateless/integration · iceberg regression (iceberg_1 + iceberg_2, x86 + aarch64) · parquet · s3_export · most alter/s3/rbac regression suites · docker/grype.

Iceberg regression green is the strongest signal that the core mutation path works in the dedicated test suite.

---

Recommendation

1. Fix test_alter_drop_column_without_reload before merge — consistent failure across build types; 0 historical passes.
2. Do not block on swarms/settings/version/kafka regression red or SQLLogic timeout.
3. Ignore test_remote_initiator failure — follow #1856.
4. After fixing the integration test, re-run CI; iceberg regression already green should not need changes.

CI verdict: Red for a real, PR-scoped integration gap (ALTER DROP COLUMN without reload), not for the bulk of regression noise.

[alsugiliazova]

Audit update for PR #1841

AI audit note: This review comment was generated by AI (claude-opus-4.7).

PR: Altinity/ClickHouse#1841 — Fixed iceberg alter table modify rename (backport of #1594)
HEAD: 889ec3f20fb25b887f2d079f2600f6a7923d595b

Previously-flagged defects, now fixed

Verified against commit 5be2aba1 ("Fix ai audit defects") and the current HEAD diff:

• Medium (prior): const_cast<DataLakeStorageSettings &> on configuration_->getDataLakeSettings() — fixed. IcebergMetadata::data_lake_settings is now const DataLakeStorageSettings & and Iceberg::alter takes a const &. No unsynchronized cross-thread mutation remains.
• Medium (prior): mutation of data_lake_settings[iceberg_metadata_file_path] = metadata_name — fixed. The write was removed in 5be2aba1.
• Low (prior): silent adoption of an existing on-storage metadata file when !wrote_ok && file_exists — fixed. The retry loop now unconditionally continues on write failure rather than adopting whatever happens to be at the target path.

Confirmed defects (still open at HEAD)

High — Orphan-cleanup removeObjectIfExists can delete the live metadata file under concurrent ALTER

• Impact: Two concurrent ALTERs on the same catalog-backed Iceberg table can both compute last_version + 1 and target the same v(N+1).metadata.json path. ALTER A writes the file and commits successfully via the REST catalog. ALTER B then writes (overwrites) the same path; the catalog rejects B (assert-current-schema-id mismatch); B's failure handler calls object_storage->removeObjectIfExists(metadata_info.path) and deletes v(N+1).metadata.json — the file the catalog now points to. The table is left broken (catalog references a deleted/inconsistent metadata file). Trigger is realistic for catalog-backed tables (multi-node ClickHouse, or ClickHouse + Spark/Trino concurrently altering the same table).
• Anchor: src/Storages/ObjectStorage/DataLakes/Iceberg/Mutations.cpp / Iceberg::alter orphan-cleanup branch (the if (!catalog->updateMetadata(...)) block — removeObjectIfExists(StoredObject(storage_metadata_name)) followed by throw).
• Trigger: Two ALTER TABLE operations on the same Iceberg REST table started before either commits; B writes after A's successful catalog commit.
• Why defect: The cleanup path assumes the just-written file belongs exclusively to this attempt. It does not verify the on-storage content matches what this attempt wrote (e.g. by checksum, ETag from PUT, or by writing to a unique temp path and renaming). Object-storage PUT-overwrite plus naïve unconditional delete = data corruption.
• Fix direction: Either (a) write metadata to a unique temp object and only rename/copy to the final path after catalog commit succeeds; or (b) capture the PUT ETag/version-id and only delete if the current object still matches; or (c) do not delete at all on catalog failure and rely on a separate orphan-sweeper.
• Regression test direction: Two-thread integration test where T1 commits v6 successfully, then T2's catalog commit is forced to fail (failpoint) for the same target version; assert v6.metadata.json still exists and catalog.load_table() succeeds.

Medium — old_schema_id = new_schema_id - 1 is brittle and breaks for non-contiguous schema IDs

• Impact: buildUpdateMetadataRequestBody derives the assert-current-schema-id requirement arithmetically. Any Iceberg table whose current-schema-id was not produced by this code's MetadataGenerator (e.g. created/evolved by Spark, or a future change to MetadataGenerator's numbering) gets a wrong requirement value and the REST catalog rejects every ALTER.
• Anchor: src/Databases/DataLake/RestCatalog.cpp / buildUpdateMetadataRequestBody — const Int32 old_schema_id = new_schema_id - 1;.
• Trigger: ALTER on a catalog-backed Iceberg table whose previous current-schema-id is not exactly new - 1.
• Why defect: The function receives the entire metadata blob; the previous current-schema-id is recoverable from the input rather than guessed. Coupling correctness to a private MetadataGenerator increment-by-one invariant is fragile and undocumented at the contract boundary.
• Fix direction: Pass the previous current-schema-id explicitly from the caller (capture before mutation in MetadataGenerator::generate*ColumnMetadata), or thread it through new_snapshot as a sibling field used only for the requirement.
• Regression test direction: gtest with snapshot containing current-schema-id=6 and a schemas array having ids {2, 6}; expect the produced requirement's current-schema-id equals the actual previous value (here 2), not 5.

Medium — checkAlterIsPossible silently no-ops when metadata is uninitialized

• Impact: Pre-PR, checkAlterIsPossible invoked assertInitializedDL() which threw a clear LOGICAL_ERROR if metadata had not loaded. The new code drops both assertInitializedDL() and BaseStorageConfiguration::assertInitialized(), falling through silently when current_metadata is null. ALTER validation is therefore skipped in that state and the operation proceeds to params.apply(new_metadata, context) and configuration->alter(...), which then throws a less-clear error from the deeper assertInitializedDL() inside alter.
• Anchor: src/Storages/ObjectStorage/DataLakes/DataLakeConfiguration.h / DataLakeConfiguration::checkAlterIsPossible (if(current_metadata) current_metadata->checkAlterIsPossible(commands);).
• Trigger: ALTER on a datalake-backed table whose first metadata load failed (transient catalog/S3 error) or before initialization completed.
• Why defect: Loss of fail-fast invariant; replaces a precise diagnostic with silent skip + later, less specific error. Also bypasses BaseStorageConfiguration::assertInitialized() even on the happy path.
• Fix direction: Restore the explicit assertInitializedDL() (or throw a dedicated "datalake metadata not initialized" error) in checkAlterIsPossible; do not gate the call on a null check.
• Regression test direction: Force metadata-load failure (failpoint or catalog returning 500), call ALTER, expect the explicit "metadata is not initialized" error rather than a downstream message.

Low — Catalog-failure orphan cleanup swallows all exceptions silently

• Impact: When removeObjectIfExists fails (permission denied, transient network), the only signal is a log line via tryLogCurrentException, then the original DATALAKE_DATABASE_ERROR is thrown. The orphan v(N+1).metadata.json remains in object storage and accumulates over repeated failures; nothing surfaces this to the user or to monitoring.
• Anchor: src/Storages/ObjectStorage/DataLakes/Iceberg/Mutations.cpp / Iceberg::alter — try { object_storage->removeObjectIfExists(...); } catch (...) { tryLogCurrentException(log, ...); }.
• Trigger: Object-storage permission/transient error during cleanup after catalog rejection.
• Why defect: Acceptable trade-off (preserve catalog-failure error), but no mechanism reclaims the orphan; existing test only covers the happy cleanup path.
• Fix direction: Append cleanup-failure detail to the thrown error message, or increment a metric/system table for orphan metadata so an external sweeper can act.
• Regression test direction: Inject failure in both catalog commit and removeObjectIfExists; assert thrown error mentions both catalog failure and cleanup failure paths.

Low — if(current_metadata) style violates spacing convention

• Impact: Style only; will likely be flagged by the style check or by reviewers.
• Anchor: src/Storages/ObjectStorage/DataLakes/DataLakeConfiguration.h / DataLakeConfiguration::checkAlterIsPossible — if(current_metadata).
• Why defect: Inconsistent with the file's if (...) spacing.
• Fix direction: if (current_metadata).

Coverage summary

• Scope reviewed: All 17 changed files at HEAD 889ec3f. Call graph: StorageObjectStorage::alter → configuration->update → params.apply → configuration->alter(params, ctx, storage_id, catalog) → DataLakeConfiguration::alter → IcebergMetadata::alter → Iceberg::alter → metadata generation + writeMetadataFileAndVersionHint + catalog->updateMetadata → RestCatalog::updateMetadata → buildUpdateMetadataRequestBody → sendRequest → orphan-cleanup branch on failure → metadata_cache->remove.
• Categories failed: concurrent-ALTER orphan-cleanup race (High); brittle schema-id derivation (Medium); validation-bypass on uninitialized metadata (Medium); silent cleanup-failure (Low); minor style (Low).
• Categories passed: REST request-body correctness on happy path (gtest covers both schema-update and snapshot-append paths, missing/mismatched current-schema-id throws, stringification valid); retry loop bounded by MAX_TRANSACTION_RETRIES; failpoint-driven happy-path orphan cleanup (test_alter_orphan_metadata_cleanup_on_catalog_failure); ADD/DROP/MODIFY column integration (no_spark); database->alterTable correctly skipped for datalake catalogs in StorageObjectStorage::alter. Three Medium/Low defects from the previous audit are confirmed fixed in commit 5be2aba1 (see "Previously-flagged defects, now fixed" above).
• Not applicable: iterator/reference invalidation, integer overflow (Int32 schema IDs incremented by 1), RAII leaks (Poco JSON Ptrs are ref-counted).
• Assumptions/limits: Static reasoning only; no runtime execution. The High finding assumes object-storage PUT semantics overwrite an existing key without rejecting (standard for S3/GCS/Azure) and that two ALTERs can race past getLatestOrExplicitMetadataFileAndVersion before either commits — both realistic for a multi-node / multi-engine catalog deployment.

[alsugiliazova]

SHOW CREATE TABLE `namespace_64f1723c_6034_11f1_bcbb_de7b9eea3490.table_64f17250_6034_11f1_bcbb_de7b9eea3490`

Query id: 01d2cf9c-5516-482a-b7d5-0b1811b71c39

   ┌─statement──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐
1. │ CREATE TABLE iceberg_database_656c372e_6034_11f1_bcbb_de7b9eea3490.`namespace_64f1723c_6034_11f1_bcbb_de7b9eea3490.table_64f17250_6034_11f1_bcbb_de7b9eea3490`↴│
   │↳(                                                                                                                                                             ↴│
   │↳    `name` Nullable(String),                                                                                                                                  ↴│
   │↳    `double` Nullable(Float64),                                                                                                                               ↴│
   │↳    `integer` Nullable(Int64)                                                                                                                                 ↴│
   │↳)                                                                                                                                                             ↴│
   │↳ENGINE = Iceberg('http://minio:9000/warehouse/data/', 'admin', '[HIDDEN]')                                                                                     │
   └────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘

1 row in set. Elapsed: 0.008 sec. 

clickhouse1 :) alter table `namespace_64f1723c_6034_11f1_bcbb_de7b9eea3490.table_64f17250_6034_11f1_bcbb_de7b9eea3490` (MODIFY COLUMN `integer` Nullable(Int64))

ALTER TABLE `namespace_64f1723c_6034_11f1_bcbb_de7b9eea3490.table_64f17250_6034_11f1_bcbb_de7b9eea3490`
    (MODIFY COLUMN `integer` Nullable(Int64))

Query id: 888d560d-d02c-463d-b553-559eea45bdc4


Elapsed: 0.028 sec. 

Received exception from server (version 26.3.10):
Code: 736. DB::Exception: Received from localhost:9000. DB::Exception: Iceberg alter: catalog commit failed for 's3://warehouse/data/metadata/v2-4472d315-2d8b-4301-8866-6d8781718fd9.metadata.json' after metadata file was written successfully. (DATALAKE_DATABASE_ERROR)

[alsugiliazova]

clickhouse1 :) alter table `namespace_64f1723c_6034_11f1_bcbb_de7b9eea3490.table_64f17250_6034_11f1_bcbb_de7b9eea3490` (MODIFY COLUMN `integer` Nullable(Int32))

ALTER TABLE `namespace_64f1723c_6034_11f1_bcbb_de7b9eea3490.table_64f17250_6034_11f1_bcbb_de7b9eea3490`
    (MODIFY COLUMN `integer` Nullable(Int32))

Query id: f8f5704b-7c2a-46d1-8c4b-8049b6bc0111


Elapsed: 0.011 sec. 

Received exception from server (version 26.3.10):
Code: 1000. DB::Exception: Received from localhost:9000. DB::Exception: Bad cast exception: Can not convert %s to %s.. (POCO_EXCEPTION)

clickhouse1 :)