[fix](mtmv) Clear stale partition state for prepared statements - #66370
[fix](mtmv) Clear stale partition state for prepared statements#66370seawinde wants to merge 3 commits into
Conversation
### What problem does this PR solve?
Issue Number: N/A
Related PR: N/A
Problem Summary: Server prepared statements reuse a StatementContext
across executions. Query partition and relation mappings from prior
executions therefore accumulate, while partition compensation allocates
a temporary BitSet for each stored RelationId. Clear the per-execution
mappings before planning each EXECUTE and query the existing BitSet
directly.
### Release note
Fixed excessive FE CPU and memory usage when repeatedly executing
prepared statements with materialized view rewrite.
### Check List (For Author)
- Test: Unit Test
- ExecuteCommandTest
- PartitionCompensatorTest
- Behavior changed: No
- Does this need documentation: No
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
FE UT Coverage ReportIncrement line coverage |
FE Regression Coverage ReportIncrement line coverage |
TPC-H: Total hot run time: 28378 ms |
### What problem does this PR solve? Issue Number: N/A Related PR: apache#49514, apache#58643 Problem Summary: The existing unit test only checked stale partition mappings after the mocked executor returned, so it could not distinguish cleanup before planning from cleanup after execution. Add a focused test that simulates current planning state being written during each EXECUTE and verifies that only the current RelationId remains. ### Release note None ### Check List (For Author) - Test: Unit Test - ./run-fe-ut.sh --run org.apache.doris.nereids.trees.plans.commands.ExecuteCommandTest - Behavior changed: No - Does this need documentation: No
|
run buildall |
TPC-DS: Total hot run time: 168879 ms |
ClickBench: Total hot run time: 23.89 s |
seawinde
changed the title
|
/review |
github-actions
Bot
added
the
approved
|
PR approved by at least one committer and no changes requested. |
|
PR approved by anyone and no changes requested. |
FE UT Coverage ReportIncrement line coverage |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I found three issues in the reused StatementContext reset: one can return stale MV rows, while the others retain stale planning/configuration state and growing optimizer work.
Critical checkpoint conclusions:
- Goal and data correctness: the allocation-free
BitSet.getrewrite is correct, and the two new clears fix their narrow maps, but the prepared-execution reset remains incomplete. A stale valid-partition cache can omit required union compensation and return stale MV rows. Retained full-plan and phase state also defeats the stated repeated-EXECUTE resource/behavior goal. - Scope and design: the patch is small and readable, but adding individual clears at the command boundary is not sufficiently complete for a context that deliberately survives executions; a centralized per-external-EXECUTE reset is needed.
- Concurrency and lifecycle: no new lock or concurrency issue was found; commands on one connection are serialized. The important lifecycle is normal prepared-context reuse. Reusable short-circuit and group-commit branches do not consume this MV state, while the non-reusable short-circuit refresh installs a fresh context.
- Configuration and compatibility: no FE/BE protocol, function symbol, storage-format, persistence, or rolling-upgrade change is introduced. Dynamic MV strategy/hint behavior and
disable_nereids_ruleschanges are affected by the retained state described inline. - Parallel paths and conditions: normal, reusable/refreshed short-circuit, group-commit, exception, ALL_PARTITIONS, empty-filter, and relation-filter paths were reviewed. The direct membership condition is equivalent to the removed singleton-BitSet intersection.
- Tests and results: the new Mockito test proves only that the two selected maps are cleared. Existing
PartitionCompensatorTestcases cover the local BitSet/sentinel behavior, but no test performs a second real planning pass with changed MV validity or retained planner state, so it misses the correctness failure and other lifecycle defects. Per the review workflow, no local build or test was run. - Observability, transactions, and performance: no transaction/persistence or FE-BE variable path is involved. The direct BitSet lookup removes avoidable allocation, but retained plans, filters, rewrite duration, cached rules, and statistics still cause incorrect accounting or growing work/memory.
There was no additional user-provided review focus. The full PR was reviewed.
| } | ||
| PrepareCommand prepareCommand = preparedStmtCtx.command; | ||
| StatementContext statementContext = preparedStmtCtx.getStatementContext(); | ||
| // Prepared statements reuse StatementContext across executions. Discard partition |
There was a problem hiding this comment.
[P2] Reset the remaining per-query planner state here
These two clears leave other state from the same planning pass alive in the reused context: materializedViewRewriteDuration accumulates until later EXECUTEs hit the rewrite cutoff; hints appends until one USE_MV/NO_USE_MV becomes a duplicate; joinFilters grows and is rescanned by CostModel; and queryStatsRecorded suppresses hit accounting after the first client execution. The cached disableRules BitSet also makes a later SET disable_nereids_rules ineffective for this prepared statement. Please make this a centralized per-external-EXECUTE reset (without resetting retry-scoped state inside one execution) and exercise an actual second planner run rather than only a mocked executor.execute().
There was a problem hiding this comment.
Fixed the MV-related part in b959429. StatementContext.resetMaterializedViewStateForPreparedExecution() is called once at the external EXECUTE boundary, resets materializedViewRewriteDuration, and removes stale UseMvHint instances while preserving unrelated hints. The test now invokes a real NereidsPlanner.plan() on both EXECUTEs.
I intentionally did not clear joinFilters, disableRules, or queryStatsRecorded in this PR. They are independent prepared-StatementContext lifecycle concerns, not involved in the partition/MV root cause, and resetting them would change join-cost, session-rule, and query-statistics behavior without dedicated coverage. They should be handled separately rather than broadening this correctness fix.
| StatementContext statementContext = preparedStmtCtx.getStatementContext(); | ||
| // Prepared statements reuse StatementContext across executions. Discard partition | ||
| // information collected by the previous execution before planning the current one. | ||
| statementContext.getTableUsedPartitionNameMap().clear(); |
There was a problem hiding this comment.
[P1] Reset cached MV-valid partitions before each EXECUTE
On the CBO/NOT_IN_RBO path, this reused context also retains mvCanRewritePartitionsMap. Suppose EXECUTE 1 records {m1,m2} as synchronized, then a write makes only m2 stale. EXECUTE 2 recomputes current validity as {m1}, which is still nonempty, but MTMVRelationManager.isMVPartitionValid uses putIfAbsent, so PartitionCompensator.calcInvalidPartitions reads the old {m1,m2}. It consequently adds neither PartitionRemover nor the base-table LogicalUnion for m2, and stale MV rows can be returned. Clear this cache at the same per-execution boundary and cover a two-EXECUTE partial-invalidation case.
There was a problem hiding this comment.
Fixed in b959429. mvCanRewritePartitionsMap is now cleared at the same per-external-EXECUTE boundary as the query partition mappings, so the next planning pass cannot read the prior {m1,m2} value through putIfAbsent and must recompute the current valid set.
The repeated-execution test seeds a stale MV-valid-partition cache, verifies it is empty before each planning pass, and runs the real Nereids planner twice. This directly covers the cache boundary responsible for the partial-invalidation failure without adding a heavyweight MTMV integration setup.
| // Prepared statements reuse StatementContext across executions. Discard partition | ||
| // information collected by the previous execution before planning the current one. | ||
| statementContext.getTableUsedPartitionNameMap().clear(); | ||
| statementContext.getCommonTableIdToRelationIdMap().clear(); |
There was a problem hiding this comment.
[P2] Reset retained MV pre-rewrite state for each EXECUTE
RecordPlanForMvPreRewrite appends a complete analyzed plan on every eligible execution, but this boundary never clears tmpPlanForMvRewrite. preMaterializedViewRewrite then walks the entire retained history on each later pass (the code even assumes the list has only one plan), while the retained preMvRewritten flag makes OptimizeGroupExpressionJob omit all MV exploration rules from the subsequent CBO phase. Repeated EXECUTEs therefore retain unbounded plan graphs and do O(n) historical work per pass while carrying the prior phase decision forward. Clear the temporary/rewritten plans, MV phase flags/masks, and relation statistics here, and add a repeated real-planning test that keeps the state bounded and validates the next execution's MV phases independently.
There was a problem hiding this comment.
Fixed in b959429. The per-external-EXECUTE reset now clears tmpPlanForMvRewrite, rewrittenPlansByMv, needPreMvRewriteRuleMasks, needPreMvRewrite, preMvRewritten, materializationRewrittenSuccessSet, and relationIdToStatisticsMap, together with the MV partition caches, rewrite duration, and stale UseMvHint entries.
The test executes a real Nereids planning pass twice on the reused prepared StatementContext; before each pass it verifies the previous MV plans, masks, flags, successes, and relation statistics are gone, then repopulates the prior-execution state to validate the second boundary independently and keep retained plan state bounded.
TPC-H: Total hot run time: 28404 ms |
TPC-DS: Total hot run time: 169380 ms |
ClickBench: Total hot run time: 23.85 s |
FE Regression Coverage ReportIncrement line coverage |
### What problem does this PR solve? Issue Number: N/A Related PR: apache#66370 Problem Summary: Server prepared statements reuse one StatementContext across external EXECUTEs. Materialized-view partition caches, pre-rewrite plans, phase decisions, hints, timing, and MV relation statistics from an earlier execution could therefore affect later planning or grow without bound. Reset only the partition and materialized-view planning state at the external EXECUTE boundary while preserving unrelated prepared-statement state. ### Release note Fixed stale materialized-view planning state across repeated prepared statement executions. ### Check List (For Author) - Test: Unit Test - ./run-fe-ut.sh --run org.apache.doris.nereids.trees.plans.commands.ExecuteCommandTest - Behavior changed: Yes (each prepared EXECUTE independently computes partition and materialized-view rewrite state) - Does this need documentation: No
github-actions
Bot
removed
the
approved
|
run buildall |
|
/review |
There was a problem hiding this comment.
Request changes.
The direct BitSet.get replacement is semantically equivalent on the planner's valid RelationId domain, and the new boundary correctly clears the listed partition/result/phase caches. Two prepared-StatementContext lifecycle gaps still leave the overall MV-state isolation incomplete: a dropped/replaced MTMV generation can remain selectable, and an enabled materialization hook can keep discovery and related-table locking active after rewrite is disabled.
Critical checkpoint conclusions:
- Goal and proof: the PR addresses retained partition/MV planning state and removes a per-entry allocation. The added tests prove reset timing and invoke a physical planner twice, but
select 1plus manually seeded mocks does not exercise either remaining discovery lifecycle. Real two-EXECUTE DROP/replacement and enabled-to-disabled regressions are still needed. - Scope and clarity:
ExecuteCommandcalls the reset at the correct external execution boundary, and thePartitionCompensatorchange is small and clear. The reset itself is broader than the PR description yet remains incomplete because candidate, related-table, and materialization-hook state is omitted. - Concurrency and locking: prepared execution is connection-serial, and no new lock-order inversion or deadlock was found. However, retained related tables expand the second execution's read-lock set; with rewrite disabled, an unrelated MV/base-table writer can still cause the one-minute planner wait or failure described inline.
- Lifecycle and error behavior: normal planning rebuilds the cleared fields; nested retries do not re-enter
ExecuteCommand, and reusable/stale short-circuit and group-commit paths do not consume the cleared MV state. Catalog DROP only marks/unregisters the old MTMV object; no downstream availability, lock, cache, scan-generation, or translation fence rejects that retained generation, which is the P1 issue below. Errors otherwise remain fail-fast. - Configuration and parallel paths: both query and DML materialization hooks eventually recheck their current enable flags, so no disabled rewrite is alleged. That check occurs after candidate discovery and table locking, leaving the P2 overhead/availability defect. Query, DML, RBO/CBO, short-circuit, group-commit, and retry paths were reviewed.
- Compatibility, persistence, writes, and protocols: no FE-BE variable, serialization/storage format, EditLog, transaction, data-write, dynamic-config definition, or rolling-upgrade surface is changed.
- Tests and results: the JUnit additions are synchronous and deterministic; existing partition tests cover empty masks, relation filters, concrete-empty sets, and ALL_PARTITIONS sentinels. This review remained static-only because the review contract forbids builds and test execution.
- Performance and observability: the direct bit lookup removes the intended temporary allocation, and clearing retained plan/result state reduces repeated work. Retained candidates/hooks still undermine the CPU, memory, and lock-footprint goal. Existing planner logging is sufficient; no new metric or log is required for this cleanup.
- User focus:
review_focus.txtcontains no additional guidance, so the complete PR was reviewed without a narrower focus.
Existing live threads were treated as known context and not repeated. After main-agent adjudication, a second full-coverage plus risk-focused round returned NO_NEW_VALUABLE_FINDINGS against the unchanged head and exact final comment set; all candidates are accepted, dismissed with evidence, or deduplicated.
| needPreMvRewrite = false; | ||
| preMvRewritten = false; | ||
| materializationRewrittenSuccessSet.clear(); | ||
| relationIdToStatisticsMap.clear(); |
There was a problem hiding this comment.
[P1] Clear retained MV candidates before rebuilding them
A dropped MTMV can still be selected on the next execution:
EXECUTE 1: ResultSink -> Scan(base) candidateMTMVs={mv_old}
DROP MATERIALIZED VIEW mv_old
EXECUTE 2: ResultSink -> Scan(mv_old [dropped, old table id])
DROP marks the old object isDropped and removes it from the live relation manager, but this reset leaves candidateMTMVs/mtmvRelatedTables alive. getAvailableMTMVs then consumes the retained set directly and checks rewrite status/partitions, not isDropped or the current catalog generation; AsyncMaterializationContext can consequently generate a scan from that old object and partition IDs. Clear candidateMTMVs, candidateMVs, and mtmvRelatedTables here so collection rebuilds them, and cover DROP or same-name replacement between two real EXECUTEs.
| commonTableIdToRelationIdToMap.clear(); | ||
| mvCanRewritePartitionsMap.clear(); | ||
| materializedViewRewriteDuration = 0; | ||
| hints.removeIf(UseMvHint.class::isInstance); |
There was a problem hiding this comment.
[P2] Remove retained MV hooks when rewrite is disabled
An enabled execution installs InitMaterializationContextHook.INSTANCE in this reused context. If the session then disables MV rewrite, AddInitMaterializationHook merely declines to add another hook; it does not remove the old one. containMaterializedViewHook therefore remains true, so CollectRelation still discovers MVs and lock() still acquires all retained MV-related table locks before initMaterializationContext finally rechecks the disabled flag. A writer holding any such lock can make a base-table EXECUTE wait for the one-minute planner timeout or fail even though MV rewrite is off. Remove only materialization hook instances here (preserving unrelated hooks) and clear their candidate/related-table state so the current collect pass can re-add them only when enabled.
TPC-H: Total hot run time: 29324 ms |
TPC-DS: Total hot run time: 167349 ms |
FE Regression Coverage ReportIncrement line coverage |
ClickBench: Total hot run time: 23.81 s |
4 participants
What problem does this PR solve?
Issue Number: N/A
Related PR: #49514, #58643
Problem Summary:
Repeated server prepared statement executions can retain query partition and
relation mappings from earlier executions. No GitHub issue is associated with
this fix; the production incident is tracked outside GitHub.
Root cause: In
ExecuteCommand.run(), server prepared statements reuse thesame
StatementContext, whileQueryPartitionCollectorappends currentpartition and relation mappings without removing mappings from prior
executions.
PartitionCompensator.getQueryUsedPartitions()then scans theaccumulated entries and creates a temporary
BitSetfor every RelationId,increasing FE CPU, allocation, and GC overhead as the connection remains active.
Change Summary:
ExecuteCommand.javaEXECUTE.PartitionCompensator.javaBitSetdirectly instead of allocating a singletonBitSetfor each mapping.ExecuteCommandTest.javaThe change resets only the two confirmed per-execution mappings and leaves
placeholder, short-circuit, connector, MVCC, and other prepared statement state
unchanged.
flowchart LR A[EXECUTE starts] --> B[Clear prior partition mappings] B --> C[Plan current execution] C --> D[Check RelationId with BitSet.get]Release note
Fixed excessive FE CPU and memory usage when repeatedly executing prepared
statements with materialized view rewrite.
Check List (For Author)
Test
./run-fe-ut.sh --run org.apache.doris.nereids.trees.plans.commands.ExecuteCommandTest,org.apache.doris.nereids.rules.exploration.mv.PartitionCompensatorTestBehavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)