Workflow [PR], commit [a064fd4]

Summary: ✅

• Performance Comparison: Performance dashboard

AI Review

Summary

This PR bumps contrib/zstd to the ClickHouse zstd clickhouse branch merge commit that vectorizes the short-offset ZSTD_wildcopy path on AArch64. I found no unresolved correctness, safety, metadata, or performance-evidence issues requiring inline findings.

Final Verdict

Status: ✅ Approve

@groeneai, provide a fix for test_storage_iceberg_concurrent.

@alexey-milovidov fix in #108075.

Root cause: test_concurrent_reads runs 25 concurrent SELECT * per round, each fanning out to ~max_threads (core count) read threads, so the readers oversubscribe the global thread pool under load. On the asan/db-disk/old-analyzer 5/6 shard one SELECT is starved past the 300s client receive_timeout (the failure here and on #100651 and master); locally it's an outright CANNOT_SCHEDULE_TASK. Pinning max_threads=4 on the SELECT bounds the demand without weakening the test — all 25 readers still race the 15 writers and the row-count assertion is unchanged. Verified both directions on a debug build.

FYI ClickHouse/zstd#4

@thevar1able Let's bring it here too please and I'll review the 2 together. We should also consider sending the patches upstream once we are ok with them

The only reason I skipped x86 is that the current testing machine is AArch64.
So, for AArch64, it is tested quite thoroughly; for x86, we don't have it.

Please do not merge x86 optimization here - better to do it in steps for more confidence.
Also, for x86, we might want to have dynamic dispatch.

The only reason I skipped x86 is that the current testing machine is AArch64.
So, for AArch64, it is tested quite thoroughly; for x86, we don't have it.

Performance PRs are tested both for x86 and ARM

Also, for x86, we might want to have dynamic dispatch.

Our baseline is v3, which is what that PR is introducing. We'd only need dynamic dispatch if there was a v4 method

Performance tests don't use zstd for now, so they are pointless here.

Anyway, this depends on #108093

📊 Cloud Performance Report

🟢 AI verdict: improvement — 3 query(s) improved out of 38 analysed

This PR bumps the zstd library and nothing else. Three scan-heavy ClickBench queries improved in lockstep (Q4 -20.6%, Q15 -18.8%, Q28 -5.7%), all confirmed by both tests over large sample sizes. A uniform, same-direction speedup across independent scans is the expected signature of faster decompression lowering CPU cost per block read. No regressions were flagged.

clickbench

🟢 3 improved

tpch_adapted_1_official

🟢 No significant changes

@groeneai, provide a fix for https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=108049&sha=1555aa62391f6e0ba72fd8c2818fe60b0f23a96a&name_0=PR&name_1=Stress%20test%20%28arm_debug%29

@alexey-milovidov This is the streaming DISTINCT-in-order logical error (Equal values are not contiguous within the range assumed to be sorted), already fixed by #108568.

Failing query: SELECT count() ... FROM (SELECT DISTINCT * FROM t_streaming_test STREAM LIMIT 10). Stack: DistinctSortedStreamTransform::transform -> ColumnString::getEqualRangeEndAssumeSorted -> the checkEqualRangeEndAssumeSorted assertion in findEqualRangeEndAssumeSorted.h (added by #106502).

Root cause: a STREAM read returns parts in commit order, not sorting-key order, so its output is not globally sorted by the sorting key. checkSupportedReadingStep accepted the streaming ReadFromMergeTree and requested read-in-order, advertising a sorting-key order that does not hold and feeding unsorted data to DISTINCT-in-order. The assertion then aborts the server in debug/sanitizer builds (handled normally in release).

#108568 ("Do not apply read-in-order optimizations to STREAM reads") guards exactly this path with if (reading->getQueryInfo().isStream()) return nullptr;, and its regression test covers the identical SELECT DISTINCT * ... STREAM LIMIT shape. On review now.

We need more data to prove this optimization. In the worst case, we will have to make it adaptive, like our LZ4_decompress_faster.

@alexey-milovidov Agreed. No-regression sweep plan, plus the adaptive fallback design.

No-regression column sweep (Graviton 4, ZSTD level 1)

Methodology mirrors src/Compression/examples/decompress_perf.cpp: real hits column bytes, each column compressed at level 1, min of N runs, baseline submodule vs patched, identical binary otherwise, MB/s per column. The PR table has 4 integer columns; the matrix expands to every type class, and the entropy-bound and incompressible columns are the ones that must stay >= 1.00x:

• Integers, all widths: Int8/16/32/64, UInt* (CounterID, UserID, WatchID, AdvEngineID, RegionID, ClientIP, ResolutionWidth, ...)
• LowCardinality: the categorical hits columns (dictionary index streams)
• Date / DateTime: EventDate, EventTime
• Float: where present
• Strings, entropy-decode-bound: URL, Title, Referer, SearchPhrase
• Incompressible: WatchID-like high-entropy columns

Pass criterion: no class below 1.00x within run-to-run noise. Strings and incompressible should be neutral since they exercise the non-overlap path and the FSE/Huffman decoder, untouched here.

I cannot run this on the fleet (x86, where the NEON path compiles out to COPY8, and x86 is the separate out-of-scope zstd PR #4). It needs the AArch64 host. Do you want me to drive it there, or will you run it on the same machine as the original table?

Correctness (done, arch-independent)

I verified the init/adv tables are byte-exact against the scalar src[k % diff] reference by simulating vqtbl1q_u8 in plain C: 125.8M output bytes across every overlap period diff=1..15 and length 1..4096, zero mismatches. So this is purely a throughput question.

Adaptive fallback, if the sweep shows a regression

Structural difference from LZ4: LZ4::decompress(..., PerformanceStatistics&) is called per block from the codec and the bandit times each block to select among decompressImpl<8/16/32>. ZSTD goes through the opaque ZSTD_decompress, and the overlap copy is ZSTD_wildcopy called per match inside the sequence decoder, so a block-level bandit is not a drop-in. Two options:

1. In-wildcopy dispatch on (diff, length), cheap, no statistics. The short-offset branch already holds both. Gate NEON behind a length threshold so the fixed setup (two vld1q plus vqtbl1q to build the pattern) never loses to COPY8 on very short runs, falling back to scalar there. This is the direct analogue of LZ4 picking copy granularity by data characteristics, and it targets exactly the regime where 16-byte tbl can regress.
2. Full LZ4-parity bandit at the CompressionCodecZSTD layer: expose a wildcopy-strategy selector in the contrib fork and thread a PerformanceStatistics accumulator through the codec. Heavier (contrib API plus codec changes), only if (1) is insufficient.

Precedent from our LZ4 NEON code: single vtbl1_u8 (8-byte) is a 6% Graviton win, but vtbl2_u8 at 16/32-byte was slower than scalar so we kept scalar there. This PR uses single-call vqtbl1q_u8 (16-byte, no in-loop load), which sidesteps the multiple-call cost that hurt LZ4, so it is plausibly a genuine 16-byte win. That contrast is also why the per-column sweep matters: 16-byte NEON tbl has been a coin-flip in this codebase.

Merged ClickHouse/zstd#3. We need to point this PR to clickhouse branch in zstd repo, maybe @groeneai can handle this?

@thevar1able The clickhouse branch HEAD is now bde4f839008a59c3689493a0903c7251988cf41d (the zstd#3 merge commit), so that is what contrib/zstd should point at.

It is a pure pointer move, zero content change: the tree of bde4f839 (b2b1f684...) is byte-identical to the current submodule 1ced1f24, and 1ced1f24 is the merge's second parent. The gitlink just re-targets the permanent clickhouse branch instead of the now-merged PR-3 feature branch, so no rebuild risk.

One line on optimize-zstd-decompression-aarch64:

git -C contrib/zstd fetch origin clickhouse && \
git -C contrib/zstd checkout bde4f839008a59c3689493a0903c7251988cf41d && \
git add contrib/zstd && \
git commit -m "Point contrib/zstd at clickhouse branch (zstd#3 merged)" && git push

I do not have push access to this branch (it is on ClickHouse/ClickHouse, not a fork I control), so I cannot push it myself. Happy to open a PR against the branch with this bump if that is easier.

Continue-PR status — CI triage (all reds are unrelated flakes)

The PR itself is ready: AI Review ✅ Approve, @Algunenano APPROVED, 0 unresolved review threads, and the latest Cloud Performance Report is a 🟢 improvement (Q4 −20.6%, Q15 −18.8%, Q28 −5.7%, no regressions flagged) — which answers the earlier "we need more data / measure other columns" concern. The contrib/zstd pointer is already correctly moved to the clickhouse-branch HEAD bde4f839 (commit "Update submodule pointer"), as @groeneai requested; its tree is byte-identical to the previously-pointed 1ced1f24.

This is a pure contrib/zstd submodule bump, so none of the CI reds can be caused by it. Triage (CIDB, last 10 days):

1. Stateless arm_asan_ubsan, azure, sequential — 4 workload/scheduler tests failed together: 03821_drop_workload_during_query_race, 03562_cpu_scheduling_with_one_thread, 03232_workloads_and_resources, 03232_workload_create_and_drop. 0 failures on master; the identical 4-test cluster failed on the same config on unrelated PR Allow EXISTS on a dictionary with the SHOW DICTIONARIES privilege #108084. Root cause is global-workload-state cross-test leakage: 03821 creates the root WORKLOAD wl_03821 and runs a DROP/CREATE race loop with a best-effort EXIT cleanup (DROP WORKLOAD IF EXISTS wl_03821 2>/dev/null ||:); when that cleanup doesn't fully take, wl_03821 survives in the shared sequential server, so a later create workload all in 03232_workloads_and_resources fails with The second root is not allowed. You should probably add 'PARENT wl_03821' clause. — exactly the observed error. A ZSTD decompression speedup cannot produce a workload-graph DDL error.

   @groeneai, this is an unrelated CI flake with no tracking issue — could you investigate and provide a fix in a separate PR? Report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=108049&sha=a064fd421a09493c119fc473ef63c94064a6ca75&name_0=PR&name_1=Stateless%20tests%20%28arm_asan_ubsan%2C%20azure%2C%20sequential%29 (the same cluster is reproducible on Allow EXISTS on a dictionary with the SHOW DICTIONARIES privilege #108084).

2. Stress test arm_msan — "Hung check failed, possible deadlock found" — known OPEN issue Hung check failed, possible deadlock found #107941 (linked in the CI report itself); 1150 hits across 442 PRs (210 on master) in 10 days. Unrelated.

3. Upgrade check amd_release — "Error message in clickhouse-server.log" — a leftover test mutation retrying if(1, CAST(plus(x, throwIf(1)),'UInt64'), x) logs FUNCTION_THROW_IF_VALUE_IS_NON_ZERO, which the log scanner flags. 89 hits across 81 PRs in 10 days — broad upgrade-check log noise, not related to this PR.

I re-ran the failed jobs (workflow run 28447611330). Not re-merging master: merge-base is from today (dd89f87, 01:22Z), and none of the reds have a master-side fix that a re-merge would pick up, so a re-merge would only churn CI. The PR is ready to merge once CI clears.

@alexey-milovidov done — fix is in a separate PR: #108978.

Confirmed your root cause, with one refinement: the leak isn't a partial cleanup, it's that the EXIT-trap cleanup never runs. On timeout clickhouse-test sends SIGTERM then SIGKILL 0.1s later; 03821's trap cleanup EXIT doesn't fire on SIGTERM and 0.1s is too short, so when 03821 times out the root wl_03821 + the (WORKER THREAD, MASTER THREAD) rs_03821 leak on the shared sequential server. CIDB shows exactly that ordering on this PR and on #108084: 03821 fails with Timeout! Processes left in process group, then 03232_workloads_and_resources / 03232_workload_create_and_drop (second root) and 03562_cpu_scheduling_with_one_thread (second WORKER THREAD resource) fail together, return code 36, 0 on master.

Fix: bound every client call in 03821 with timeout so a query/DDL that wedges under the drop/create race can't block wait until the 180s cap (where SIGKILL skips cleanup). The test then finishes normally and the EXIT trap always drops the workload + resource. Verified locally: a leaked wl_03821/rs_03821 deterministically reproduces both sibling errors; patched test passes 50/50 under randomization with zero leftover state.

Continue-PR status — CI triage (only red is an unrelated stress flake)

The PR remains ready to merge:

• AI Review ✅ Approve, @Algunenano APPROVED, 0 unresolved review threads.
• Latest Cloud Performance Report is a 🟢 improvement (Q4 −20.6%, Q15 −18.8%, Q28 −5.7%, no regressions flagged) — answering the earlier "we need more data / measure other columns" concern.
• The contrib/zstd pointer is correctly on the clickhouse-branch HEAD bde4f839 (commit "Update submodule pointer" a064fd42); zero content change vs. the previous 1ced1f24.

Only CI red on a064fd42: Stress test (arm_msan) → Logical error: Bad cast from type A to B (STID: 1499-32c3). This is an unrelated, broadly-flaky stress failure of the well-known testing,fuzz "Bad cast from type A to B" class (sibling open issues #108931 / #107598 / #103814 for other STIDs of the same class). CIDB for this exact STID over the last 10 days shows 4 occurrences across 4 distinct PRs and 3 different stress configs (amd_tsan, arm_msan, arm_debug), all on master / base_ref=master (06-22, 06-29, 06-30) — i.e. it reproduces independently of any PR. A pure contrib/zstd submodule bump (NEON vectorization of the short-offset decompress wildcopy) cannot produce a type-cast assertion deep in the pipeline executor, so this is not caused by this PR. Reran the failed job (run 28447611330 --failed).

The workload/scheduler flake cluster flagged in the previous pass is now fixed in @groeneai's #108978 — thanks.

No re-merge: merge-base dd89f87 is from today (06-30 01:22Z, < 1 day), and no master commit since then touches contrib/zstd or src/Compression, so a re-merge would pull in nothing relevant and would only re-churn CI on a master-wide flake.

Blocked only on the stress rerun going green and a maintainer merge.

Continue-PR status — CI is now fully green, ready to merge

The stress rerun this PR was blocked on has passed. Current state of a064fd42:

• CI: 0 failures, 0 pending — 155 SUCCESS, 19 SKIPPED in the rollup, including CH Inc sync and Style check. The Stress test (arm_msan) "Bad cast from type A to B" (STID 1499-32c3) that was red at 20:18Z re-ran green — as triaged, it is the master-wide testing,fuzz cast-assertion flake and cannot be produced by a pure contrib/zstd bump.
• AI Review ✅ Approve, @Algunenano APPROVED, 0 unresolved review threads.
• Cloud Performance Report 🟢 improvement (Q4 −20.6%, Q15 −18.8%, Q28 −5.7%, no regressions) — answers the earlier "we need more data / measure other columns" concern.
• MERGEABLE / CLEAN, no conflicts.

No re-merge: merge-base dd89f87 is ~1.3 days old and no master commit since touches contrib/zstd, .gitmodules, or src/Compression, so a re-merge would pull in nothing relevant.

Blocked only on a maintainer merge.