Decryption support and encryption-header cache for the experimental ReaderExecutor#109702
Merged
Conversation
The experimental ReaderExecutor (gated by `use_reader_executor`, default off) so far fell back to the legacy read path whenever a file was encrypted. Teach it to decrypt served payload itself so it engages on encrypted disks too. A new `ReaderExecutorDecryptor` (under `USE_SSL`) parses the stacked encryption headers once and decrypts in place; it is reentrant (a fresh per-call CTR `Encryptor` per layer), so no shared mutable state. The executor keeps its logical `position` and derives the physical source offset as `position + data_start_offset` (the header bytes at the file front), makes `totalSize` logical, and decrypts each freshly-read window at its logical offset before serving plaintext. `ReadPipeline` no longer treats a decryption stage as unsupported: it feeds the layers to the executor and calls `initDecryption`. Adds the `ReaderExecutorDecryptMicroseconds` profile event, gtests (single- and multi-window, multi-layer, undersized-file saturation, long-connection EOF slot release, reentrant decrypt) and a functional test reading a MergeTree table on an encrypted no-cache object-storage disk (`04508`), with a new `s3_no_cache_encrypted` storage policy in the stateless config.
On the experimental ReaderExecutor path, `initDecryption` read the encryption
header (`N * Header::kSize` bytes) from the source on every open of an encrypted
file — a network GET per column-stream open. Add a process-global cache of the
raw header bytes, keyed by the file's storage path, so repeated opens of the same
file skip that read.
The cache stores opaque bytes (not parsed `FileEncryption::Header`s), so it has no
SSL dependency and compiles in every build; on a hit the executor re-parses the
tiny header. It is a `CacheBase` (SLRU), sized by the new
`encryption_header_cache_size` server setting (default 50 MiB, hot-reloadable) and
cleared by `SYSTEM DROP ENCRYPTION HEADERS CACHE` (also cleared with the other
path-keyed caches, since a reused path could otherwise return stale headers).
Only disk reads populate/consult it: `DiskEncrypted::prepareRead` marks the
pipeline via `ReadPipeline::allowEncryptionHeaderCache`, and the executor receives
the cache only then — url / external-storage reads never cache, so a reused
external path can never serve stale headers.
Adds `CurrentMetrics::EncryptionHeaderCache{Bytes,Entries}`,
`ProfileEvents::EncryptionHeaderCache{Hits,Misses}`, a gtest and a functional test
(`04509`), and the `SYSTEM_DROP_ENCRYPTION_HEADERS_CACHE` access type.
Contributor
|
Workflow [PR], commit [2dd1e2c] Summary: ❌
AI ReviewSummaryThis PR extends the experimental Final Verdict✅ No new findings. |
The style check requires any test using SYSTEM DROP to carry the no-parallel tag, because it clears a process-global cache shared with other parallel tests.
server_settings_to_string dereferences getEncryptionHeaderCache() to report the cache size, but clickhouse-local never created it, so querying system.server_settings segfaulted. Initialize a dummy cache in LocalServer, mirroring the query condition cache.
Drop the provenance wording ('isolated helper extracted from ReaderExecutor')
and references to concepts not present in this change (the transient sub-executor,
prefetch workers); state the class's behaviour and thread-safety instead.
The decryptor's per-layer buffer_size was stored but never read: the executor reads the header sized by data_start_offset, not buffer_size (that field sizes the legacy ReadBufferFromEncryptedFile, still fed to wrapDecryption). Drop it from the decryptor layer and the executor's addDecryptionLayer. path stays -- it is the key finder's path_for_logs argument for key-resolution diagnostics.
Drop hypothetical and defensive-counterfactual asides (a reused path 'could' return stale headers, url reads 'even if encrypted', read-ahead 'served later'), past-behavior notes, and restatements of obvious code; keep the non-obvious why.
![clickhouse-gh[bot]](https://avatars.githubusercontent.com/in/928874?s=60&v=4){kind=small}
The header cache is keyed by the file's object remote_path. For backends that reuse a path across rewrites (s3_plain_rewritable, local/web encrypted disks) a cached header would decrypt freshly-written ciphertext with a stale IV. Add EncryptionHeaderCache::drop and call it from DiskEncrypted on write(Rewrite) / replace / remove / truncate, so a sequential read -> rewrite -> read of a reused path re-reads the new header. This closes the sequential path-reuse case. A concurrent in-place rewrite of a file being read is not closeable by drop alone, but the executor only reads immutable, ref-counted MergeTree part data (never rewritten under a live reader) -- the same assumption the mark and uncompressed caches already make. Adds a gtest (read -> rewrite same path -> read) proving the stale read and its fix via drop.
parseHeaders resolves each layer's key from its fingerprint via key_finder and stores it in layer.key; the finder is unused afterwards. Clear it once the key is resolved so no key-resolution callback is retained past initialization.
CheSema
marked this pull request as ready for review
July 8, 2026 08:57
Replace the per-operation cache invalidation with a gate: DiskEncrypted enables the header cache only when its delegate uses random object keys (metadata-based object storage, `isRemote() && !isPlain()`). There every write produces a new `remote_path`, so a rewrite / replace / rename never rebinds an existing key to different ciphertext -- the cache is self-invalidating and needs no explicit `drop`. Deterministic-path backends (plain / plain-rewritable, local, web), where the key is reused on rewrite, are excluded. This removes the invalidation hooks in DiskEncrypted::write/replace/remove/truncate (and EncryptionHeaderCache::drop). Those hooks ran on every encrypted-disk write regardless of use_reader_executor and broke test_encrypted_disk; the gate keeps the write path untouched and closes the stale-header class of bugs by construction.
The isRemote() && !isPlain() approximation wrongly included web disks, which are remote and non-plain but assign deterministic blob paths (areBlobPathsRandom() == false) — a URL replaced in place could serve a stale cached header. Use the exact predicate delegate->areBlobPathsRandom() so only backends that assign a fresh blob path per write enable the cache.
Add a section to system.md for the new cache-clear statement, alongside the other DROP CACHE commands.
Add a gtest reading one encrypted file (header + ciphertext) split across two StoredObjects, so the executor's position + data_start_offset shift and the findObjectAt boundary crossing are exercised while decrypting -- prior encryption tests used a single object.
Add a doubly-encrypted no-cache storage policy (DiskEncrypted over DiskEncrypted over raw object storage) and a functional test reading a MergeTree table on it under use_reader_executor=1, asserting correct data and that the executor's multi-layer decryption path engages end-to-end.
Contributor
|
Preview deployment for your docs. Learn more about Mintlify Previews.
|
kssenii
approved these changes
Jul 9, 2026
…Settings Per review: the executor-only cache doesn't belong in the broadly-shared ReadSettings. DiskEncrypted::prepareRead now fetches the global cache (Context::getGlobalContextInstance) and passes it to ReadPipeline::needEncryptionHeaderCache, mirroring needLongConnectionLimit; the pipeline forwards it to the executor Options. Removed the ReadSettings field.
- totalSize(): make the empty-source case explicit and assert the non-empty invariant (physical == 0 -> 0; else chassert(physical >= data_start_offset)) instead of a saturating subtraction that implied an impossible 'partial header' case; a partial file throws at initDecryption. Split the gtest into empty-source (returns 0) and undersized-throws cases. - rename position_phys/object_phys_start_offset -> *_physical. - rename decryptInPlace -> decryptInPlaceIfNeeded (it no-ops when there is no encryption).
Adding CLEAR_ENCRYPTION_HEADERS_CACHE (SYSTEM DROP ENCRYPTION HEADERS CACHE) pushed the last enumerator RESET_DDL_WORKER from ordinal 127 to 128, outside magic_enum's default reflection range [-128, 127]. ParserSystemQuery matches SYSTEM keywords via magic_enum::enum_values, so RESET_DDL_WORKER silently dropped out of the value list: SYSTEM RESET DDL WORKER stopped parsing and its access check was never reached, making 04401_system_reset_ddl_worker_access fail with both the unprivileged and on-cluster queries reported as NOT denied. Specialize magic_enum::customize::enum_range<ASTSystemQuery::Type> to cover every value (min = 0, max = 512), matching the fix used by other recent SYSTEM-command additions (e.g. #109639).
CheSema
enabled auto-merge
July 10, 2026 11:34
Contributor
LLVM Coverage Report
Changed lines: Changed C/C++ lines covered: 475/500 (95.00%) · Uncovered code |
robot-ch-test-poll
added
the
pr-synced-to-cloud
1 task
This was referenced Jul 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of the effort to land the experimental
ReaderExecutorread path as aseries of small, independently-reviewable PRs (see #103706). Everything here
stays gated behind the experimental
use_reader_executorsetting (default off).This PR teaches the executor to read encrypted files:
Decryption support. The executor no longer falls back to the legacy read
path on encrypted files — it decrypts served payload itself via a new,
reentrant
ReaderExecutorDecryptor(position-addressable CTR, decrypted inplace at each window's logical offset).
positionstays logical; the physicalsource offset is
position + data_start_offset(the encryption headers at thefile front).
Encryption-header cache. A process-global cache of the raw encryption
header bytes, keyed by the file's storage path, so repeated opens of the same
encrypted file skip re-reading the header from the source. Sized by the new
encryption_header_cache_sizeserver setting (default 50 MiB, hot-reloadable)and cleared by
SYSTEM DROP ENCRYPTION HEADERS CACHE. Only disk reads (stable,engine-managed paths) populate or consult it; url / external-storage reads
never cache.
Tests: IO gtests for the decryptor and the executor decryption/cache paths, plus
functional tests reading a MergeTree table on an encrypted no-cache disk and
exercising the new SYSTEM command.
Related: #103706
Related: #102282
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
The experimental
ReaderExecutorread path (use_reader_executor) now supports reading encrypted files, with a global encryption-header cache configured by theencryption_header_cache_sizeserver setting and cleared bySYSTEM DROP ENCRYPTION HEADERS CACHE.Documentation entry for user-facing changes
Version info
26.7.1.757(included in26.7and later)