Repository navigation
Conversation
A server-supplied FixedString(0) creates a ColumnFixedString with a zero width, and ColumnFixedString::Size() divides by it as soon as Block::AppendColumn() sees the zero-row header block. A malformed width such as FixedString(-) makes std::stol throw out of TypeParser::Parse(). ParseTypeName() inserts the cache entry before parsing and erases it only when Parse() returns false, so the throw leaves a half-built AST cached and the next query with the same type hits the same division. Number tokens now parse with std::from_chars, and invalid or out-of-range values fail the parse instead of throwing, which lets the existing erase path run. ParseTypeName() also parses into a local and caches only on success, so an exception from the parser can no longer leave an entry behind. The factory rejects sizes outside 1..0xFFFFFF, the server's own DataTypeFixedString limit (MAX_FIXEDSTRING_SIZE), and the ColumnFixedString constructor rejects 0. The server never accepts FixedString(0), so the tests that built one now use FixedString(1).
The 1..0xFFFFFF FixedString check only ran in CreateTerminalColumn(). With low_cardinality_as_wrapped_column set, CreateColumnFromAst() builds LowCardinality(FixedString(N)) straight from the AST value, so a server could declare any width and the client would allocate it on the first load. I moved the check into GetFixedStringSize() and call it from both paths.
ParseTypeName() keeps every distinct type name it parses in a process-wide map with no eviction, so a server that sends many distinct type names, one zero-row block each, grows client memory without bound. The cache now holds at most kTypeAstCacheMaxEntries entries and kTypeAstCacheMaxBytes of type-name text. An insert that would exceed either limit clears the cache first, and a name longer than the byte budget is parsed on every call without being cached. The cleared map is destroyed after the mutex is released. Clearing would invalidate ASTs that callers are still using, so ParseTypeName() now returns std::shared_ptr<const TypeAst> instead of a pointer into the map. This changes a function declared in an installed header; the in-tree caller compiles unchanged.
iliaal
force-pushed
the
fix/type-ast-cache-bound
branch
from
October 18, 2026 21:16
7dd08c0 to
cf7ea8c
Compare
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
1 participant
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.
Builds on #577. Only the last commit, "Bound the parsed type AST cache", is new here.
ParseTypeName()intypes/type_parser.cppkeeps every distinct type name it parses in a process-wide map with no eviction. A server can send many distinct type names, one zero-row block each, and grow client memory without bound: 100,000 uniqueFixedString(N)names add about 72 MB RSS.Fix: the cache holds at most
kTypeAstCacheMaxEntries(4096) entries andkTypeAstCacheMaxBytes(256 KiB) of type-name text. An insert that would exceed either limit clears the cache first. The cleared map is destroyed after the mutex is released. A name longer than the byte budget is parsed on every call and never cached. Repeated types are still reused.API change: once entries can be evicted, a raw pointer into the map can dangle in another thread, so
ParseTypeName()now returnsstd::shared_ptr<const TypeAst>instead ofconst TypeAst*. The in-tree caller (CreateColumnByType, which usesauto) compiles unchanged. External code that stores the result in aconst TypeAst*fails to compile, rather than silently holding a pointer with a new lifetime rule. Keeping the raw pointer would mean never freeing entries. Returning by value would deep-copy the AST for every column of every block, undoing the point of the cache.Tests:
ParseTypeName.CacheReusesAst,CacheEntryCountIsBounded,CacheByteBudgetIsBoundedandOversizedNameIsNotCached.