[DNM] executor: optimize range scan for cached table#67430
Closed
you06 wants to merge 48 commits into
Closed
Conversation
Cache c.GetType(ctx) into a local variable in Constant.EvalInt and Constant.EvalReal to avoid redundant method calls. Also remove the sleep 1200 between work file iterations in the cached table scan run script. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
For Constant expressions without ParamMarker, directly access RetType to get the flag instead of calling GetType(sctx), reducing overhead in integer comparison operations. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…located FieldType
Add getTypeNonAlloc that fills a caller-provided FieldType buffer on the
stack for ParamMarker constants instead of allocating via NewFieldType.
Replace all GetType calls in Eval{Int,Real,String,Decimal,Time,Duration,
JSON,VectorFloat32} with this non-allocating variant to reduce GC pressure
in hot paths.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
For cached tables, getSnapshotRow() always returns nil, making the merge logic in getOneRow() unnecessary overhead. Add a dedicated nextForCacheTable() method that reads directly from addedRowsIter, skipping the snapshot row merge and reducing per-row cost. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Introduce memRowsBatchIterForTable for cached table scans. It decodes rows into chunks and applies vectorized filters when enabled. Add a benchmark comparing row vs batch iter.
Add a specialized decoder for fixed-length int-handle row keys to reduce overhead in mem table scans, and add tests/bench updates.
- Add wide-row decode and mixed-append benchmarks\n- Optimize chunk fixed-len appends and fixed AppendNull\n- Cache per-column decode metadata in ChunkDecoder\n- Add correctness tests and cached-table-scan work plans
Pass buf and preAlloc parameters through to decodeIndexKvGeneral and decodeIndexKvForClusteredIndexVersion1 so callers (DecodeIndexKVEx) can reuse pre-allocated slices. Replace CutIndexKeyNew with CutIndexKeyTo to write into the pre-allocated result slice directly and use reEncodeHandleTo instead of reEncodeHandle to append in-place. Remove the now-unused reEncodeHandle helper. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ue decoding Add IndexRestoredDecoder to cache the colIDs map and BytesDecoder across rows, avoiding per-row allocations in decodeRestoredValues. Introduce arena-based encodeOldDatumToArena and DecodeToBytesNoHandleInto to further reduce allocations during index KV decoding in the general path. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add Time.AppendString() that formats date/datetime/timestamp via direct byte writes instead of fmt.Sprintf and string concatenation, eliminating allocations. Rewrite Time.String() to delegate to AppendString. In DumpTextRow, use AppendString with the existing tmp buffer to avoid hack.Slice(t.String()) allocations. Also bump tmp buffer from 20 to 32 bytes to accommodate the longest datetime+fsp string. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Cache HandleStatus, time.Location, and EvalContext in memIndexReader so they are computed once per scan instead of on every row decode. Also use DecodeColumnValueWithDatum to avoid intermediate allocations. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Introduce a per-cached-table result set cache that stores query result sets keyed by plan digest and parameter hash. The cache is invalidated when the table lease expires, avoiding stale reads without eviction logic. Includes unit tests for hit/miss, capacity limits, and concurrency safety. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Ensure the cached table result set cache is properly invalidated when data may become stale: on data reload, lease renewal failure, and before acquiring a write lock. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add CanCacheResultSet to determine whether a query's result set can be served from the cached table result set cache. The check rejects queries with non-deterministic functions (NOW, RAND, UUID, etc.), user/session variable references, FOR UPDATE locks, DML context, and non-cached tables. Includes comprehensive unit tests covering all rejection paths. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add BuildResultCacheKey to construct cache keys from plan digest and parameter hashes, enabling per-query result set caching for cached tables. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…d tables Wire up the result set cache by wrapping top-level executors with CachedResultExec. Move ResultCacheKey to the table package so it can be referenced across executor and planner without import cycles, and expose GetCachedResult/PutCachedResult on the CachedTable interface. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add metrics (hit/miss counters, memory gauge, eviction counter), slow log field (Result_cache_hit), EXPLAIN ANALYZE runtime stats, and the StmtCtx.ReadFromResultCache flag to provide visibility into result set cache behavior on cached tables. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…e fixes Add secondary paramBytes verification on cache lookups to guard against FNV-1a hash collisions. Fall back to OriginalSQL hashing when plan cache parameterization is bypassed (e.g. non-prepared plan cache disabled). Exclude PointGetPlan and BatchPointGetPlan from result set caching since they bypass plan cache parameterization. Remove obsolete workplan files. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add memCachedDatumIter that reads pre-decoded chunks from CachedDatumData, skipping KV decode entirely. Wire it into memTableReader.getMemRowsIter and UnionScanExec.handleCachedTable for cached table scans. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Store TIMESTAMP columns in UTC in datum cache with tracked indices for per-session timezone conversion. Add timezone-aware test for cached table result set cache. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Cover projection, desc scan, filters, timezone conversion, multi-chunk desc, and benchmark for the cached datum fast-path iterator. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Cherry-pick of 1ee4b3c from PR #66640 (Part 5). Adds CachedIndexDatumData and BuildCachedIndexDatumData for pre-decoding index KV entries into datum maps at cache load time. The executor builder now looks up index datum caches via GetCachedIndexDatumData interface and populates indexDatumCaches on UnionScanExec.
… condition checks Cherry-pick of def0558 from PR #66640 (Part 5). - Reuse us.mutableRow field instead of allocating a new MutRow per Next() call in both Next and nextForCacheTable. - Add direct-to-chunk datum write fast path in nextForCacheTable when no virtual columns or conditions exist. - Skip EvalBool when conditions slice is empty in memIndexReader.getMemRows and memRowsIterForIndex.Next. - Reuse mutableRow in memCachedDatumIter instead of allocating via MutRowFromDatums on every iteration. Note: removeRedundantAccessConditions was already cherry-picked in Part 4.
Cherry-pick of 57df0aa from PR #66640 (Part 5). - Add kvRangesCoverFullTable guard to only use datum cache fast path when the scan covers the full table record range and txn membuffer has no overriding entries. - Add safety checks in CachedResultExec for schema match validation. - Add comprehensive tests for datum cache fallback and backfill edge cases.
|
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
ti-chi-bot
Bot
added
the
do-not-merge/release-note-label-needed
|
Review failed due to infrastructure/execution failure after retries. Please re-trigger review. ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
Bot
added
the
size/XXL
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Hi @you06. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (67)
📝 WalkthroughWalkthroughAdds a result-set caching subsystem: planner eligibility and key builders, a CachedResultExec executor wrapper, table-level result and pre-decoded datum/index caches, executor integration (builder/union-scan/mem-reader), metrics/slow-log/runtime-stats, many decoder/rowcodec optimizations, and extensive tests/benchmarks. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ExecAdapter as Exec Adapter
participant Builder as Executor Builder
participant CRE as CachedResultExec
participant Cache as CachedTable
participant Src as Wrapped Executor
Client->>ExecAdapter: Execute(query)
ExecAdapter->>Builder: build(plan)
Builder->>Builder: CanCacheResultSet(...) & BuildResultCacheKey(...)
Builder->>CRE: wrapWithResultCache(exec, stmtNode, plan)
CRE->>Cache: GetCachedResult(key, paramBytes)
alt hit
Cache-->>CRE: chunks + fieldTypes, ok=true
CRE->>CRE: schemaMatch -> set hit state, StmtCtx.ReadFromResultCache=true
CRE-->>Client: Next() returns cached rows
else miss
Cache-->>CRE: miss
CRE->>Src: Open() (delegate to wrapped exec)
Src-->>CRE: Next() returns rows
CRE->>CRE: collect chunks while Next()
Client->>CRE: Close()
CRE->>Cache: PutCachedResult(key, paramBytes, collectedChunks, schema)
end
sequenceDiagram
participant Planner
participant KeyBuilder
participant Session
participant StmtCtx
Planner->>Planner: CanCacheResultSet(stmt, plan, inDML)
alt cacheable
Planner->>KeyBuilder: BuildResultCacheKey(sctx)
KeyBuilder->>StmtCtx: get plan digest
KeyBuilder->>Session: get timezone, charset, collation
KeyBuilder->>KeyBuilder: encode params, build verification payload, compute ParamHash
KeyBuilder-->>Planner: ResultCacheKey + paramBytes
else not cacheable
Planner-->>Planner: skip result cache
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
you06
force-pushed
the
perf/cached-table
branch
from
70e9ff3 to
a930d09
Compare
|
@you06: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
[FORMAT CHECKER NOTIFICATION] Notice: To remove the 📖 For more info, you can check the "Contribute Code" section in the development guide. Notice: To remove the For example:
📖 For more info, you can check the "Contribute Code" section in the development guide. |
Contributor
Author
|
Dev branch in the wrong repo... |
|
Caution Review failedAn error occurred during the review process. Please try again later. 📝 WalkthroughWalkthroughAdds result-set caching for cached-table queries: planner eligibility/key builders, a new CachedResultExec executor, in-memory result cache and pre-decoded datum/index caches, executor/builder integration and metrics/slow-log/runtime-stats, plus many supporting decoding, iterator, and test changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Executor as Exec Adapter
participant Builder as Executor Builder
participant CRE as CachedResultExec
participant Cache as CachedTable
participant Reader as MemReader
Client->>Executor: Execute(query)
Executor->>Builder: build(plan)
Builder->>Builder: CanCacheResultSet(...) & BuildResultCacheKey(...)
Builder->>CRE: wrapWithResultCache(exec, stmtNode, plan)
CRE->>Cache: GetCachedResult(key, params)
alt hit
Cache-->>CRE: chunks + fieldTypes, ok=true
CRE->>CRE: schemaMatch -> set hit state, StmtCtx.ReadFromResultCache=true, increment hit metric
else miss
Cache-->>CRE: miss
CRE->>Reader: Open() (delegate)
Reader->>Reader: use pre-decoded datum/index caches or KV scan
CRE->>CRE: collect chunks while Next()
Client->>CRE: Close()
CRE->>Cache: PutCachedResult(key, params, collectedChunks, schema)
CRE->>CRE: increment miss metric
end
Client->>CRE: Next() -> returns rows (either copied from cache or delegated)
sequenceDiagram
participant Planner
participant KeyBuilder
participant Session
participant StmtCtx
Planner->>Planner: CanCacheResultSet(stmt, plan, inDML)
alt cacheable
Planner->>KeyBuilder: BuildResultCacheKey(sctx)
KeyBuilder->>StmtCtx: get plan digest
KeyBuilder->>Session: get timezone, charset, collation
KeyBuilder->>KeyBuilder: encode params, hash (ParamHash)
KeyBuilder-->>Planner: ResultCacheKey + paramBytes
else not cacheable
Planner-->>Planner: skip result cache
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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
2 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.
Summary
Test plan
make serverbuilds successfully🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
tidb_plan_cache_policysession variable to control plan caching behavior with "hint_only" modeResult_cache_hitfield to slow query logsImprovements
Metrics