Skip to content

Stable 25.8 Backport of #109551 - Fix crash in Iceberg due to type confusion - #2539

Merged
mkmkme merged 4 commits into
stable-25.8from
backports/stable-25.8/109551
Oct 16, 2026
Merged

mkmkme merged 4 commits into
stable-25.8from
backports/stable-25.8/109551

Conversation

@mkmkme

@mkmkme mkmkme commented Oct 10, 2026

Copy link
Copy Markdown
Collaborator

Fix crash in Iceberg due to type confusion

Changelog category (leave one):

  • Critical Bug Fix (crash, data loss, RBAC)

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

Fix a crash when reading Iceberg tables with equality delete files. If a column is nullable in the equality delete file but non-nullable in the table schema (or vice versa), the values read from the delete file were inserted into a column of a different type through an unchecked cast (a column type confusion), corrupting the column and crashing the server (ClickHouse#109551 by @mstetsyuk)

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)

…lity-delete-nullability

Fix crash in Iceberg due to type confusion
@mkmkme mkmkme added backport Backport 25.8 25.8 Altinity Stable stable-25.8 labels Oct 10, 2026
@github-actions

github-actions Bot commented Oct 10, 2026 •

Copy link
Copy Markdown

Workflow [PR], commit [e95a4f6]

@mkmkme

mkmkme commented Oct 15, 2026

Copy link
Copy Markdown
Collaborator Author

@blau-ai

@blau-ai

blau-ai commented Oct 15, 2026

Copy link
Copy Markdown
Collaborator

CI triage for #2539

Verdict - 11 red checks: 7 PR-caused (all the same test), 2 infra/flaky, 1 process (DCO), 1 aggregate gate.

Check Class Cause
6x Stateless tests (...) + Stateless ... flaky check PR-caused New test 03595_equality_deletes_simple fails deterministically (flaky check: 50/50 fail)
Stress test (amd_debug), Stress test (amd_ubsan) infra Server failed to start - Azure "Connection refused" at metadata load; unrelated to this diff
DCO process Backported commit has no Signed-off-by
PR - Aggregate gate; goes green once the above are resolved

1. The real failure: the new test fixture does not resolve on stable-25.8 (PR-caused)

Every failing stateless suite fails on only one test - 03595_equality_deletes_simple, which this PR adds - and the amd_asan, flaky check ran it 50 times, 50 failures. That is deterministic, not flaky. The failing query is the new sub-case:

SELECT data FROM icebergS3(s3_conn, filename = 'deletes_db/eq_deletes_required_table') ORDER BY data;
Code: 499. DB::Exception: Failed to get object info: No response body.. HTTP response code: 404:
while reading se/default/eq_deletes_required_table/metadata/snap-667782107838695408-1-373fb659-...avro. (S3_ERROR)

Note the mangled key se/default/... - the file actually lives at deletes_db/eq_deletes_required_table/metadata/.... The production fix (nullability/type in IcebergMetadata.cpp) is never reached; the query dies earlier, while loading the snapshot's manifest-list.

Root cause - the fixture bakes local Hadoop paths that this branch cannot translate. The new eq_deletes_required_table was generated with a local warehouse (HadoopTables(...).load("/out/warehouse/default/...")), so its metadata stores filesystem paths:

// metadata/v3.metadata.json
"location":      "/out/warehouse/default/eq_deletes_required_table",
"manifest-list": "/out/warehouse/default/eq_deletes_required_table/metadata/snap-667782107838695408-...avro"

The existing, working eq_deletes_table fixture instead uses the S3 layout:

"location":      "s3a://test/deletes_db/eq_deletes_table",
"manifest-list": "s3a://test/deletes_db/eq_deletes_table/metadata/snap-...avro"

On stable-25.8, path translation goes through getProperFilePathFromMetadataInfo() (src/Storages/ObjectStorage/DataLakes/Iceberg/Utils.cpp:170). Its correct branch requires table_location.ends_with(common_path):

  • working table: table_location = s3a://test/deletes_db/eq_deletes_table ends with common_path = deletes_db/eq_deletes_table -> resolves to deletes_db/eq_deletes_table/metadata/...
  • new table: table_location = /out/warehouse/default/eq_deletes_required_table does not end with common_path = deletes_db/eq_deletes_required_table

So it drops into the fallback at Utils.cpp:201-203:

auto prefix = table_location.substr(0, table_location.size() - common_path.size()); // = "/out/warehou" (12 chars)
return std::string{data_path.substr(prefix.size())};                               // = "se/default/..."

48 - 36 = 12; data_path.substr(12) of /out/warehouse/default/... is exactly se/default/eq_deletes_required_table/metadata/snap-...avro -> the observed 404. The arithmetic reproduces the error byte-for-byte.

Why it is green upstream but red here: upstream ClickHouse/ClickHouse#109551 ships the same fixture, but upstream master replaced getProperFilePathFromMetadataInfo with a path_resolver abstraction (persistent_table_components.path_resolver.resolve(filename) in StatelessMetadataFileGetter.cpp) that handles absolute non-S3 paths. stable-25.8 does not have that refactor, so the backport carried a fixture its target branch cannot consume.

Suggested fixes (pick one):

  • Regenerate the fixture against the S3 layout (recommended). Produce eq_deletes_required_table so every baked-in path uses s3a://test/deletes_db/eq_deletes_required_table, exactly like eq_deletes_table - i.e. point the Iceberg warehouse at s3a://test/deletes_db (or write through the same catalog the existing fixture used) instead of the local /out/warehouse. Note: a text find/replace on the *.metadata.json is not enough: the snap-*.avro manifest-list and *-m0.avro manifest files are deflate-compressed and embed /out/warehouse/... paths too (the manifest->manifest and manifest->parquet references hit the same fallback and still 404). Those Avro files must be regenerated. Update the recipe in data_minio/deletes_db/README.md accordingly.
  • Backport the upstream path-resolver that superseded getProperFilePathFromMetadataInfo. Correct but a much larger, riskier change for a stable branch - probably out of scope for a backport.
  • Minimal unblock: keep the C++ fix but drop the new eq_deletes_required_table sub-case (the fixture, the two added .sql/.reference lines, the README section). The production fix still lands; it just is not exercised on 25.8 until the fixture is regenerated.

I cannot produce the regenerated fixture here - it needs Spark + the Iceberg Java API, which this container does not have (no build/test infra). The binary Avro/Parquet regeneration needs a human or a CI job.

2. Stress test (amd_debug, amd_ubsan) - infra

Both report check_status.tsv doesn't exists because the server never started. application_errors.txt and run.log show, on every restart attempt:

<Error> Application: Caught exception while loading metadata: Azure::Storage::StorageException,
e.what() = 500 ... Code 111, Connection refused
...
Failed to start server   (after 60 retries, NETWORK_ERROR Connection refused localhost:9000)

This is the Azure blob emulator being unreachable at startup - environmental, no Iceberg/03595/sanitizer signatures in either log. Not caused by this diff. Safe to re-run.

3. DCO - process

The PR's single commit (fac84ee8c213, the upstream merge commit) carries no Signed-off-by trailer, so the DCO app fails. Fix by adding a sign-off (git commit --amend -s / git rebase --signoff on the backport branch) or using the maintainer DCO override. No code impact.


Bottom line: the backport's C++ change looks fine and is not what is failing - the blocker is the new Iceberg test fixture, which encodes local /out/warehouse paths that stable-25.8's resolver cannot map to S3. It needs to be regenerated with s3a://test/deletes_db/... paths (or the sub-case dropped from the backport). Stress failures are infra (re-run); DCO just needs a sign-off.

How would you like me to proceed on the fixture - open a blau/* PR that drops the eq_deletes_required_table sub-case (unblocks CI, keeps the fix), or hold while someone regenerates the fixture? I can also prep the DCO sign-off if useful. I will not push anything until you choose.

Match the upstream 26.3 backport by relocating the fixture under
default/ and updating the query to match the embedded table location.
This lets the older Iceberg path resolver locate the fixture files.

Verified the focused test and 25 randomized runs.

Signed-off-by: Mikhail Koviazin <github@mkmk.aleeas.com>
@mkmkme

mkmkme commented Oct 16, 2026

Copy link
Copy Markdown
Collaborator Author

@blau-ai

@blau-ai

blau-ai commented Oct 16, 2026

Copy link
Copy Markdown
Collaborator

CI triage

3 red checks: 0 PR-caused, 2 infra, 1 process (DCO). The actual code change is healthy — the test this PR adds (03595_equality_deletes_simple) passed in every stateless suite that ran (amd_debug parallel/sequential, amd_asan, amd_ubsan, amd_binary, …).

1. Stress test (amd_debug) — infra, not PR-related

info: check_status.tsv doesn't exists, and test_results.tsv has a single row:

Cannot start clickhouse-server	FAIL
 [ 1611 ] {} <Error> Application: Caught exception while loading metadata: std::exception.
 Code: 1001, type: Azure::Storage::StorageException, e.what() = 500 Poco::Exception.
 Code: 1000, e.code() = 111, Connection refused ...

The server could not reach the Azure blob emulator (Azurite) at startup and repeatedly failed to boot (pids 1611, 2513, 3411, 4314, 5204, …). No test query ever executed, so the harness produced no check_status.tsv. This has nothing to do with the change (Iceberg equality-delete type handling, S3-only — no Azure code path touched).
→ Safe to re-run.

2. Stress test (amd_ubsan) — infra, not PR-related

Identical signature, same cause, different pids — Azurite Connection refused on every startup attempt from 00:04 to 00:06. Same verdict.
→ Safe to re-run.

3. DCO — process, PR-related (not a code problem)

DCO is ACTION_REQUIRED because the backported upstream commit is missing a sign-off trailer:

fac84ee8c21  signoff:[]  Miсhael Stetsyuk  "Merge pull request #109551 from ClickHouse/fix-iceberg-equality-delete-nullability"
169d46338b2  signoff:[Mikhail Koviazin ...]  "Fix equality-delete regression fixture path on 25.8"   (signed)

169d463… is signed off correctly; fac84ee… (the cherry-picked upstream content) carries no Signed-off-by line, which trips DCO.

Fix (author action — this rewrites that commit, i.e. a force-push to backports/stable-25.8/109551, which I won't do on your behalf):

git rebase --signoff origin/stable-25.8   # adds Signed-off-by to the unsigned commits
git push --force-with-lease

Alternatively, if this repo has the DCO app's "Set DCO to pass" override enabled, a maintainer can use that, since the content is a verbatim upstream backport.


Bottom line: nothing to fix in the code or tests. Re-run the two stress jobs (infra flake), and add a sign-off to fac84ee… to clear DCO.

@mkmkme
mkmkme merged commit 0d79d87 into stable-25.8 Oct 16, 2026
230 of 236 checks passed
@mkmkme mkmkme added the verified Approved for release label Oct 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

25.8 25.8 Altinity Stable 25.8.33.10001 backport Backport stable-25.8 verified Approved for release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants