[feature](ivm) Add the per-partition refresh state and its journal channel - #68193
Merged
Merged
Conversation
…annel Rebuilding only the MV partitions that a silent base-table change really invalidated needs a per-partition state: which generation of data the partition holds and which generation it must hold, persisted so that it survives a restart. This adds that state and the journal channel that carries it. Nothing decides anything from it yet and nothing but a replay writes it, so the behaviour of every MV is unchanged. Key changes: - Add MTMVPartitionState, a persisted refreshEpoch / latestEpoch pair keyed by MV partition name, and MTMVPartitionState.copyOf for taking a detached snapshot of a state map - Add MTMV.partitionStates with its getter and its replay setter; gsonPostProcess initializes it, so an image written before the field existed and a non-IVM MV both load as an empty map - Carry the state in the ADD_TASK payload under the same condition as ivmInfo, which keeps the journal of a non-IVM MV byte-for-byte what it was, and apply it on replay only when the field is present - Add MTMVAlterOpType.ALTER_PARTITION_STATES and its Alter.processAlterMTMV branch, so a live state change has an op of its own instead of riding on ALTER_IVM_INFO, whose branch only swaps the IvmInfo object Unit Test: - MTMVTest: an image round trip of a state, an image without the field, the getter before any state exists, the detached journal payload, the ADD_TASK carry for an IVM MV and its absence for a non-IVM MV, and both replay directions - AlterMTMVTest: replay of ALTER_PARTITION_STATES through Alter.processAlterMTMV on a real MV
yujun777
requested review from
gavinchou,
luwei16,
morrySnow and
mymeiyi
as code owners
September 18, 2026 09:35
yujun777
marked this pull request as draft
September 18, 2026 09:36
yujun777
marked this pull request as ready for review
September 18, 2026 09:36
yujun777
marked this pull request as draft
September 18, 2026 09:37
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
yujun777
marked this pull request as ready for review
September 18, 2026 09:37
Contributor
Author
|
run buildall |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Static review conclusion: request changes for two P2 issues: the new partition-state API exposes mutable owned state after releasing its lock, and the new journal channel is not tested across its actual AlterMTMV.write/read representation.
Critical checkpoint conclusions:
- Goal and proof: the change is focused on adding inert per-partition epoch state plus image, ADD_TASK, and dedicated alter replay channels. The production serialization path is structurally present, but the journal wire/restart contract lacks a round-trip oracle.
- Scope and clarity: the seven changed FE files are cohesive and do not add refresh/invalidation policy. The additive DTO and replay branches are otherwise small and clear.
- Concurrency and locking: task completion snapshots and enqueues under
mvRwLock, the FIFO edit-log queue preserves order, andawait()is correctly outside the lock. No new lock-order or deadlock issue was found. However,getPartitionStates()returns the live map and mutable values after unlocking, bypassing that ownership invariant for the follow-up callers this API is meant to support. - Lifecycle: old images initialize the missing field to empty; old ADD_TASK records with no member preserve current state; an explicit empty map clears it; replay applies detached copies. INSERT OVERWRITE keeps the formal partition name while changing its id, and no current production logic populates or consumes the map. Future sync drop/add logic must own state removal/reinitialization atomically.
- Configuration and initialization: no configuration item, static-initialization dependency, or new thread is introduced.
- Compatibility: the image and ADD_TASK fields are additive Gson members; non-IVM ADD_TASK payloads remain unchanged; the dedicated enum operation is not emitted by live code in this PR. No FE-BE protocol or storage-format change applies.
- Parallel paths and conditions: IVM/non-IVM, image, ADD_TASK success/failure replay, and dedicated alter replay were traced. The IVM-only live carry condition is consistent with this PR's stated scope.
- Tests and results: the added unit tests cover image compatibility, detached copies, absent ADD_TASK state, IVM/non-IVM carry, and in-memory replay, but not the actual alter-journal bytes or the absent-versus-empty distinction after deserialization. No result files changed. Per the runner instruction, this review was static-only: I did not run builds or tests. CheckStyle had passed externally; FE UT and compile were still pending when inspected, so they are not treated as independent validation here.
- Observability: because the state is intentionally inert in this PR, no new logging or metric is required yet.
- Persistence/failover and data correctness: detached copying before asynchronous serialization, fatal edit-log failure behavior, dispatch, and replay ordering are otherwise correct. There is no direct user-data write or transaction-protocol change.
- Performance: full-map copying is linear but currently empty/inert and is necessary for a detached payload; no separate confirmed performance defect was found.
- User focus and completion: no additional user focus was supplied. Two complete review rounds were performed; all normal and risk-focused reviewers returned
NO_NEW_VALUABLE_FINDINGSin the convergence round, and every candidate was accepted, deduplicated, or dismissed with evidence.
Contributor
TPC-H: Total hot run time: 28309 ms |
Contributor
TPC-DS: Total hot run time: 153758 ms |
Contributor
ClickBench: Total hot run time: 23.98 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
The partition-state API handed out the map the MV owns: a caller could add or change an entry after the lock was released, while the task result copies that same map into the journal, and a replay that replaced the field left the caller's reference pointing at state the MV no longer owned. The journal channel had no test that went through the representation a restart actually reads either, so a member that failed to serialize would have kept every replay test green. Key changes: - Return an unmodifiable detached snapshot from MTMV.getPartitionStates, under the read lock rather than the write lock, since it no longer needs to initialize the field - Ignore a null payload in alterPartitionStates, so a journal without the member leaves the states alone instead of clearing them; the dedicated alter op applied null as an empty map before, while the task result replay had always checked - Stop the getter from turning "no state yet" into an empty map: the task result carries the field, and a member that is present but empty means "clear" on replay, which could wipe a mark that landed between the payload and the replay Unit Test: - MTMVTest: the getter's snapshot rejects writes and its values are copies, the ADD_TASK payload survives its JSON round trip, and states are seeded through the replay setter rather than the getter - AlterMTMVTest: replay driven by the journal bytes (AlterMTMV.write / read) for a payload with state, a payload that empties it, and an older payload without the member, plus the serialized member name
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
TPC-H: Total hot run time: 27290 ms |
Contributor
There was a problem hiding this comment.
Static review completed for exact head 14e5372 against base 934233d. Recommendation: approve; no new blocking issue was substantiated at this head.
The PR goal is met within its stated scope: it adds the per-partition state DTO and a persistence/replay channel without changing refresh or visibility decisions. The MV getter returns a detached deep snapshot under mvRwLock, replay replacement and live ADD_TASK payload capture copy state under the write lock, and edit-log submission preserves the lock-acquisition order. No live ALTER_PARTITION_STATES producer or state consumer exists in this head, so deferred invalidation/cleanup behavior is not introduced here.
Persistence and compatibility checkpoints are covered: old MTMV images normalize an absent field to an empty map; old AlterMTMV journals omit the new member and replay preserves existing state; present-empty and present-nonempty payloads round-trip distinctly; the enum is Gson name-serialized. The dedicated replay test drives AlterMTMV.write/read for nonempty, empty, and absent payloads, and MTMV tests cover image round trips, detached ownership, ADD_TASK payload serialization, and non-IVM gating.
Critical checkpoint conclusions: no new configuration, static-initialization, RPC/protocol, transaction, or data-visibility path is introduced; lock ownership and error/absent-payload handling are explicit; the change is focused and does not add an observable hot-path concern. The only test-shape concern found by a subagent is the same ADD_TASK wire-round-trip point already present in discussion_r4045710021, so it was not duplicated as a new inline comment. The other existing ownership thread is likewise treated as addressed by this head.
There is no additional user-provided review focus. This was a static review only; no builds or tests were run per the review instructions.
Contributor
TPC-DS: Total hot run time: 152448 ms |
Contributor
ClickBench: Total hot run time: 23.96 s |
Contributor
Author
|
run cloud_p0 |
morrySnow
approved these changes
Sep 21, 2026
luwei16
approved these changes
Sep 21, 2026
gavinchou
approved these changes
Sep 21, 2026
morningman
added a commit
that referenced
this pull request
Sep 23, 2026
…on PRs from master in merge order (#67347 #68193 #68336 #68170 #68180) (#68405) Cherry-picked from #67347, #68193, #68336, #68170, #68180 Batch pick of every merged PR carrying the `incremental-computation` label that `branch-incremental-computation` does not have yet (no `incremental-computation-picked` label), in the order they landed on master (`git log --first-parent`). One commit per PR, each ending with `(cherry picked from commit <master sha>)`. Follows the same convention as #67830, #68017, #68073, #68151, #68236 and #68303. | # | Master commit | PR | Title | |---|---|---|---| | 1 | a7c0a78 | #67347 | [fix](cloud) bind the packed slice location lifetime to its writer | | 2 | addf0c8 | #68193 | [feature](ivm) Add the per-partition refresh state and its journal channel | | 3 | e885b46 | #68336 | [refactor](ivm) Rename IvmInfo.refreshVersion to sequencePrefix | | 4 | 01efbca | #68170 | [fix](ivm) Fall back to complete refresh when the IVM stream is unusable | | 5 | d6c1a2b | #68180 | [fix](ivm) Choose IVM baseline rebuild partitions from the MV partition mapping | Not included on purpose: - The 29 labelled PRs that already carry `incremental-computation-picked` (every other closed PR with the label). This batch closes the selection query: 34 closed labelled PRs = 29 already picked + these 5. ### Prerequisite check - **The IVM series (#68193, #68336, #68170, #68180)** — one series by the same author, merged 2026-09-21/22, all tracing to issue #65418. #68193 says in its own description that it "adds the state that the following PRs need" — `MTMVPartitionState` plus its journal channel — and #68336 is the rename of the field sitting next to it. The PRs that consume that state are in this batch too, so the series is carried whole and in master merge order. Its earlier PRs (#67802, #67837, #67814, #68138, #67646, #67669, #67575) were picked in the previous rounds. - **#67347** is standalone (cloud packed-file slice lifetime). Every file that uses the APIs it changes is in the pick; `be/src/io/fs/packed_file_system.cpp`, which also reads the global slice index but is not modified, compiles unchanged against the picked headers. - **No unpicked master commit is required by any of the five.** Verified by compiling, not by inspection alone: the whole FE main + test tree compiles and the touched unit tests pass (below), and every BE/cloud file the picks touch passes a `-fsyntax-only` compile with the real build's flags. - The unlabelled master commits touching the same files (#67186 Hive partition batching, #66530 external scan task reuse, #66761 TIMESTAMP_NS, #67545 DLF, #67067 eager-agg) are *not* prerequisites — see the drift check: none of the picks' own added lines reference what they introduce. ### Drift check against master Per pick, `git show` of the branch commit against the master squash, with `index`/`@@` lines stripped: #67347, #68193 and #68336 are identical. Two differ, both mechanically: - **#68170 / `MTMVTask.java`** — master's context around `executePartitionBasedRefresh` carries #67186's `try` block and snapshot preload; this branch does not have #67186, so the auto-merge kept the branch's block. The pick's own added and removed lines are identical to master's. - **#68180 / `MTMV.java`** — master's squash also adds `import org.apache.doris.datasource.mvcc.MvccSnapshot;`, for #67186's `pinnedSnapshots` overloads of `calculatePartitionMappings` / `getEffectiveQueryUsedBaseTablePartitionMap`. Those overloads do not exist here and the pick's new code only calls the single-argument form, which exists on both sides, so the import is not needed and not carried. For every file this batch touches, `git diff upstream-apache/master -- <file>` was taken and each master-side line attributed to the unpicked commit that added it. All 42 files resolve: 37 have no master-side difference at all, and the 5 that do are fully owned by | file | master-side lines | owner | |---|---|---| | `MTMV.java` | 20 | #67186 | | `MTMVTask.java` | 46 | #67186, #66530 | | `MTMVTaskTest.java` | 44 | #67186 | | `MTMVPartitionUtil.java` | 65 | #67186, #67545 | | `MTMVRelatedPartitionDescSyncLimitGenerator.java` | 3 | #66761 | i.e. nothing belonging to the picks is missing, and no unlabelled commit has to come along. ### Verification - FE: `run-fe-ut.sh --run` on this branch (regenerates thrift/protobuf, compiles fe-core main 4480 files + test 1485 files) over the 11 touched test classes — `AlterMTMVTest` 25, `MTMVTest` 23, `MTMVTaskTest` 48, `MetaLockUtilsTest` 6, `IvmBaselineRebuildTest` 37, `IvmAggDeltaHandlerTest` 33, `IvmDeltaRewriteStateTest` 10, `IvmFailureReasonTest` 1, `IvmInfoTest` 6, `IvmSequenceCalculatorTest` 4, `DatabaseTransactionMgrTest` 20 — **213 tests, 0 failures, 0 errors, 0 skipped, BUILD SUCCESS**. - FE checkstyle on fe-core: 0 violations. - BE/cloud: `-fsyntax-only` with the Release flags of the real build and its own compiler (`/opt/homebrew/opt/llvm@20/bin/clang++`) on `be/src/io/fs/packed_file_manager.cpp`, `be/src/io/fs/packed_file_writer.cpp`, `be/src/cloud/cloud_rowset_writer.cpp` and `be/test/io/fs/packed_file_manager_test.cpp` (the last with `-DBE_TEST -fno-access-control`): no errors. - The three new regression suites parse (`test_ivm_baseline_marker_scope`, `test_ivm_chained_stream_scope`, `test_ivm_partitions_fallback_stream_unusable`); their `.out` files are the upstream ones, unmodified. --------- Co-authored-by: Xin Liao <liaoxin@selectdb.com> Co-authored-by: yujun <yujun@selectdb.com>
yujun777
added a commit
to yujun777/doris
that referenced
this pull request
Sep 23, 2026
…of through a barrier Issue Number: N/A Related PR: apache#68170, apache#68180, apache#68193 Trace issue: apache#65418 Problem Summary: An IVM MV keeps rows that a metadata-only base-table change (DROP / TRUNCATE / REPLACE / RECOVER PARTITION) has made unusable, because such a change emits no row binlog. The invalidation used to be recorded at MV granularity -- IvmInfo.completeBaselineRebuildRequired / pendingBaselineRebuildPartitions plus a schemaChangeVersion guard -- which is coarse: one dirty partition drags the whole MV to a COMPLETE refresh, a task result produced before the invalidation is discarded, and a strict INCREMENTAL is rejected until a COMPLETE has run. This replaces the barrier with a per-MV-partition requirement: * MTMV.partitionStates maps each MV partition to {refreshEpoch, latestEpoch} (persisted as `pst`, journaled through ALTER_PARTITION_STATES). A partition is dirty iff latestEpoch > refreshEpoch. A refresh reads the requirement per batch before touching base tables and publishes only refreshEpoch once that batch's data is committed, so an invalidation arriving mid-refresh is not swallowed and a partition whose data transaction committed without its epochs being published is still rebuilt rather than read as clean. * An invalidation that can be placed on the MV partitions reading the changed base partitions raises only their requirement; every other partition keeps catching up incrementally. Alignment runs after partition sync and before any base table is read, so a mark always has an entry to land on. * The refresh routes on the criterion: dirty partitions are rebuilt by the partition executor (under a strict INCREMENTAL request as well), the rest are caught up incrementally. When every partition needs a rebuild, or the MV is in SCHEMA_CHANGE, the refresh runs as COMPLETE. A refresh that rebuilt partitions the request did not ask for reports how many in IvmRebuiltPartitions. * A whole-MV invalidation -- a change that cannot be placed on any partition, and a property change that widens what the MV maintains -- goes through the MV state instead of the barrier flag, and the barrier fields, the refresh-time guard and the pending-rebuild rejection are gone. A rename of an IVM MV's base table no longer moves it into that state, and the dependency mapping moves with the rename so a later metadata-only change to the new name still finds the MV. * A stream that has to be reconciled before a rebuild is made a real requirement on the partitions ahead of the reconciliation, so a crash between creating the replacement stream and publishing the rebuild cannot let the next refresh add its historical rows to the old baseline again. * A base-table change that only narrows excluded_trigger_tables no longer invalidates the snapshot or the version: the rows the MV holds stay valid, and an excluded table's changes are not applied, including the ones that arrived before it was excluded. A strict REFRESH ... INCREMENTAL that meets an invalidated baseline no longer fails: it rebuilds (the whole MV for a schema-level invalidation, the invalidated partitions otherwise) and reports the count in IvmRebuiltPartitions. An excluded base table's changes are not applied, including those that arrived before it was excluded -- run a COMPLETE refresh if you need them. - Test: Unit Test / Regression test - FE unit tests: IvmBaselineRebuildTest, MTMVTaskTest, MTMVTest, MTMVPlanUtilTest, CreateMTMVCommandTest, MTMVRelationManagerTest, AlterMTMVTest, IvmInfoTest, MTMVRefreshSnapshotTest, MTMVPartitionUtilTest, MetaLockUtilsTest -- all pass. - Regression: mtmv_p0/ivm -- all 97 suites pass with the stored expectations compared, not regenerated. - Behavior changed: Yes (see the release note) - Does this need documentation: Yes -- recorded in the doc-changes note that accompanies this work.
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
6 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.
What problem does this PR solve?
Trace issue: #65418
This PR adds no behaviour of its own. It adds the state that the following PRs need, and the channel
that persists it, so that they can be reviewed as logic alone.
An IVM materialized view has to invalidate the MV partitions that a base-table change really affected.
A partition drop / truncate / replace / recover changes the base table through metadata and emits no
row binlog, so the affected MV partitions must be rebuilt; today the only answer the MV has is "rebuild
all of them", which throws away partitions that are still correct.
Deciding per partition needs a per-partition answer to two questions:
That pair is
MTMVPartitionState { refreshEpoch, latestEpoch }, one entry per MV partition, keyed bypartition name.
latestEpochis the requirement,refreshEpochis the reality, and a partition whoserequirement is ahead of its reality is dirty: it holds rows read before a change that left no binlog,
so it can no longer be maintained incrementally and has to be rebuilt. The requirement has to survive a
restart, because an invalidation that only lives in memory is lost the moment the FE restarts, and a
partition that is then refreshed incrementally keeps the stale rows forever with no error anywhere.
So this PR adds
MTMVPartitionState, plus a copy helper for taking a detached snapshot), andrecord, a dedicated alter op with its replay branch, and the replay handling of the task result.
Nothing in the FE decides anything from the state yet, and nothing but a replay ever writes it, so every
MV behaves exactly as before. That is deliberate: it makes this step independently mergeable and
independently testable, which is what the PR that starts using the state needs underneath it.
Scope
MTMVPartitionState, thepartitionStatesfield, the alter record field, the new alter op and its replay branchIvmInfo; the refresh pathThe state is on the MV rather than inside
IvmInfo, and the alter op is its own rather than riding onALTER_IVM_INFO, whose branch only swaps theIvmInfoobject. Both are structural: the same state ismeant to serve a non-IVM MV later, and its journal payload must not be reconstructed as a side effect of
replaying some other op.
Key changes
MTMVPartitionState, a persistedrefreshEpoch/latestEpochpair keyed by MV partition name, andMTMVPartitionState.copyOffor taking a detached snapshot of a state map. A partition gets a new id on every refresh, so the name is the only identity it can have.MTMV.partitionStateswith its getter and its replay setter;gsonPostProcessinitializes it, so an image written before the field existed and a non-IVM MV both load as an empty map.ivmInfo, which keeps the journal of a non-IVM MV byte-for-byte unchanged, and apply it on replay only when the field is present, so an old journal applies nothing rather than clearing the state.MTMVAlterOpType.ALTER_PARTITION_STATESand itsAlter.processAlterMTMVbranch.Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
🤖 Generated with Claude Code