Conversation
…te-back
MTMV.partitionStates landed without a producer: the per-partition criterion it exists for is being added
now, starting with the state machine that keeps it true. Alignment makes an entry and its MV partition the
same thing, a refresh captures the requirement in force before it reads a base table and writes it back
afterwards, and the write-back carries only the refreshed side of the state so a requirement raised while
the task ran cannot be swallowed.
Nothing reads the criterion yet, so this changes no refresh behaviour.
Key changes:
- MTMV gains alignPartitionStates (create {0, 1} for a partition without an entry, drop entries whose partition is gone, journal the difference -- the entry has to be durable before the rows it describes), getLatestEpochs (the capture) and the write-back, applied from addTaskResult under the MV write lock
- MTMVPartitionState gains initial(), isDirty() (latestEpoch > refreshEpoch && refreshEpoch != 0) and isNeverRefreshed()
- MTMVTask captures each batch's partitions before that batch reads a base table and records them only once the batch has committed, so a batch that did not write cannot claim data
- Alignment runs after partition sync, and after the sync of the MV_PARTITION_NOT_FOUND retry: before anything reads a base table, which is what makes an invalidation always have an entry to land on
- Create partitionStates and ivmInfo with the MV so that no reader needs a null case, and keep the one load case that remains: an image carrying either member as null, which gsonPostProcess() fills
Unit Test:
- MTMVTest covers the criterion, alignment (never rewrites an existing entry, no journal when nothing changed, no-op for a non-IVM MV), the capture, the write-back recording refreshEpoch alone, a replay applying the payload's states rather than the task's captured epochs, and both load paths of the state map
- testAlignPartitionStatesCreatesAndDropsEntries, testTaskResultRecordsTheCapturedEpochWithoutTouchingTheRequirement and testPartitionStatesImageThatCarriesTheFieldAsNullLoadsAsAnEmptyMap fail when the corresponding change is reverted
The criterion the previous commit keeps now decides what a refresh does with each partition: one that holds rows read before a base-table change is rebuilt, one that is merely behind is caught up incrementally, and one that is current is left alone. Nothing invalidates a partition yet, so the dirty set is empty in a running system and this changes no refresh behaviour -- the routing is the place the invalidation will land. Key changes: - MTMV.getDirtyPartitions gives the partitions that have to be rebuilt, intersected with the partitions the MV has, and keeps the allocation and the name snapshot outside the read lock - The incremental attempt rebuilds them first, through the partition executor, and leaves them out of its own scope: the delta path can only append, so treating one as current would record it in the epoch while its rows are exactly what the rebuild replaces - The rebuild's snapshots and completed partitions are merged back after the incremental attempt reset the accumulators, so the partitions it rebuilt are not refreshed again on every following round - Escalate to COMPLETE when every partition either needs a rebuild or was never filled, and at least one needs a rebuild: COMPLETE then does nothing the routing would not, in one read of the MV - Decide the attempts after partition sync and alignment, which is what makes the partition set the escalation reads final Unit Test: - MTMVTaskTest covers the escalation, the chain it keeps when a partition is already filled, the absence of an escalation without an invalidated partition, and that the incremental attempt leaves a rebuilt partition out of its scope - MTMVTest.testDirtyPartitionsAreTheRefreshedOnesBehindTheirRequirement covers the selection, including a partition the MV no longer has - Each of the three assertions fails when the corresponding change is reverted
…ild barrier An invalidation that can be placed on the partitions reading the changed base partition now raises their requirement instead of recording a barrier the next refresh has to consume: those partitions are rebuilt, every other partition keeps catching up incrementally, and no task result is discarded for a change that never touched it. The partitions it marks also lose their refresh snapshot, which is what keeps transparent rewrite away from rows the rebuild has to replace. An invalidation that cannot be placed still takes the whole-MV route, and the barrier is still what carries it there. Key changes: - MTMV.invalidateIvmBaseline marks the selected partitions: latestEpoch raised under the MV write lock, plus the removal of their snapshots, in one journal record -- AlterMTMV gains removedSnapshotPartitions, and the ALTER_PARTITION_STATES replay applies both halves in one lock acquisition so no reader sees the new requirement while the snapshot is still there - MTMVRefreshSnapshot.removeSnapshots drops the named partitions and keeps the rest of the map, which the write-back side cannot express - A task result writes back the snapshot only for the partitions that are clean after its epochs were applied: an invalidation that reached a partition while the task ran must not have its removal undone - The partition-level invalidation no longer bumps schemaChangeVersion: a partial invalidation does not discard a task result, and the requirement it raises survives the write-back by construction Unit Test: - IvmBaselineRebuildTest pins the raised requirement per partition, that no other partition is marked, and the dropped snapshot - MTMVTest.testTaskResultLeavesTheSnapshotOfADirtyPartitionOut pins the write-back filter; MTMVRefreshSnapshotTest and AlterMTMVTest pin removeSnapshots and its replay - Each assertion fails when the corresponding change is reverted
… a barrier A rebuild requirement now has exactly two carriers, both of them already read by the refresh: a partition that reads the changed base partition carries it as a raised latestEpoch, and a change that cannot be placed on any partition carries it as the MV's SCHEMA_CHANGE state. The persisted baseline barrier (IvmInfo.completeBaselineRebuildRequired / pendingBaselineRebuildPartitions) and the handshake that consumed it (the task's pending-baseline rejection) have nothing left to carry, and the partition-level invalidation no longer bumps schemaChangeVersion either: a partial invalidation does not discard a task result, and the requirement it raises survives the write-back by construction. Key changes: - IvmInfo loses completeBaselineRebuildRequired and pendingBaselineRebuildPartitions with their accessors; the requirement lives on MTMV, as a partition's latestEpoch or as the MV state - MTMV.invalidateWholeMv is the state-machine route: the fallback that cannot place a change, and a property change that invalidates the baseline, both put the MV into SCHEMA_CHANGE, which the refresh reads - An IVM MV in SCHEMA_CHANGE refreshes as [COMPLETE] whatever the request asked for -- a schema-level invalidation is not a set of dirty partitions, it covers the partitions partition sync has not created yet. Non-IVM MVs keep the chain they had. A rename of an IVM MV's base table no longer puts it into that state, which would have it rebuild everything for nothing after the table is renamed back: the rename's own failure is already reported by the refresh, which resolves the base tables from the query first. A column change still moves the state, since telling a referenced column from an unreferenced one is the shared hook's criterion and is left as it is - MTMVTask drops the pending-baseline handshake (handlePendingIvmBaselineRebuild, validateIvmBaselineBeforePartitionSync) and MTMV drops persistIvmBaselineGuard / releaseIvmBaselineRebuild; validateIvmRefreshStart keeps only the schemaChangeVersion check - The task records how many partitions a refresh rebuilt although the request did not ask for them (ivmRebuiltPartitions, the IvmRebuiltPartitions column of the mv task TVF), so a strict INCREMENTAL that had to rebuild the whole MV reports it instead of reporting the rows as current Unit Test: - MTMVTaskTest pins C2's four boundaries: the IVM escalation, a non-IVM MV keeping its chain, an explicit partition list never widened, and a strict INCREMENTAL rebuilt and reported - IvmBaselineRebuildTest asserts each invalidation on the carrier that now exists -- the raised requirement where it can be placed, the MV state where it cannot -- and that a renamed base table leaves an IVM MV's requirement and state alone, while a non-IVM MV still gets the state - Each of the four C2 cases fails when the branch, or one of its two guards, is removed; each rename case fails when the exclusion is removed, and the non-IVM one fails when the exclusion stops being IVM-only
…nvalidation changes An IVM refresh that meets an invalidated baseline no longer refuses to run a strict INCREMENTAL request: the partitions the base-table change left behind are rebuilt as the refresh's own work, a whole-MV invalidation rebuilds the MV whole, and a refresh that rebuilt partitions the request did not ask for reports how many in IvmRebuiltPartitions. The suites that pinned the refusal now pin the outcome, and one new suite pins the routing end to end. Key changes: - test_ivm_partition_epoch_rebuild (new): one MV pins a COMPLETE baseline, an incremental refresh that rebuilds nothing and still applies its delta, a truncated base partition rebuilt by a strict INCREMENTAL while the other partition catches up incrementally and the rebuild is reported as 1, a rename of the base table that leaves both the requirement and the MV state alone, and a second truncation naming its own partition - test_ivm_baseline_marker_scope, test_ivm_partition_baseline_rebuild_dup_keys, test_ivm_chained_mtmv_2: the task rows read SUCCESS with the rebuild reported instead of the pending-rebuild failure, and the MV rows are unchanged - test_ivm_strict_failure_partition_atomicity is renamed to test_ivm_strict_incremental_rebuilds_invalidated_partitions, with its file, its expectation file and its table names: what it witnesses is a strict refresh that rebuilds, not one that fails, and its MV is asserted against the base tables' rows - The queries that read IvmFallbackReason or RefreshMode fold the unset value: an unset column comes back as the literal two-character string "\N", which does not survive the .out round trip Unit Test: - The targeted suite set (8 suites: the new one, the two chained ones, the two baseline-rebuild ones, the marker-scope one, the partition-sync-limit one and the renamed one) passes with the stored expectations compared, not regenerated - Every changed expectation is a task-route line; the MV row expectations are unchanged except where the base tables' rows are gone, which is what the rebuild is for
MTMV.snapshotsOfCleanPartitions keeps a task result from writing back the snapshot of a partition an invalidation reached while the task ran. The javadoc explained that case but not the non-IVM one, where the partition state map is never maintained: every entry has no state to be dirty in, which is why a non-IVM MV's write-back is unchanged. Key changes: - The javadoc of snapshotsOfCleanPartitions says which MVs the narrowing applies to Unit Test: - Comment only, no behaviour change; checkstyle reports no violation on fe-core
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
|
run buildall |
1 similar comment
|
run buildall |
TPC-H: Total hot run time: 28042 ms |
|
Codex automated review failed and did not complete. Error: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at Sep 28th, 2026 2:03 AM. The selected account is excluded until 2026-09-28T02:03:00Z. Please trigger /review again; another configured account may be available. |
TPC-DS: Total hot run time: 153075 ms |
ClickBench: Total hot run time: 24 s |
…hanges ### What problem does this PR solve? Issue Number: N/A Related PR: apache#68390 Problem Summary: The refresh change in this PR replaces a refusal with an escalation, and three suites pinned the refusal. A strict INCREMENTAL refresh that meets an invalidated baseline no longer fails with "IVM baseline rebuild is pending": it rebuilds, so these cases have to assert the rebuild and the data it produces rather than the error it used to report. * test_ivm_partition_drop_live_delta: the strict refresh now succeeds and consumes the surviving partitions' delta in the same run, so the case pins the MV against the base table right after that refresh, not only after the FALLBACK refresh that used to perform the repair. * test_ivm_partition_window_remove: removing ivm_partition_window_limit puts the MV in SCHEMA_CHANGE, so the strict refresh is escalated to a COMPLETE refresh and replays the p1 backlog itself; the tag that asserted the MV was left stale is now the one asserting it caught up. * test_ivm_drop_referenced_column_baseline_rebuild: dropping a referenced column still fails, because the MV query can no longer be analysed, but the same-name re-add now makes the escalated refresh succeed. The schema-ABA step asserts SUCCESS with RefreshMode COMPLETE and the rebuilt rows, instead of a "baseline rebuild is pending" rejection -- rebuilding is what keeps that case from being accepted by the incremental path with rows computed under the old column semantics. ### Release note None ### Check List (For Author) - Test: Regression test - `mtmv_p0/ivm`: the three suites above pass with the stored expectations compared, not regenerated. `test_ivm_partition_window_remove` folds RefreshMode because an unset value comes back as `\N`, which does not survive the .out round trip. - Behavior changed: No (test expectations only) - Does this need documentation: No
…y position ### What problem does this PR solve? Issue Number: N/A Related PR: apache#68390 Problem Summary: `MTMVPlanUtil#ensureMTMVQueryUsable` re-analyses the MV's query and compares the result with the MV's physical schema (`checkColumnIfChange`), position by position. The two lists are ordered by different passes: the physical schema is laid out when the MV is created, where `applyIvmPhysicalKeyLayout` puts the final key columns first and the visible key prefix is derived from the identity key slots, while the re-analysis runs that same layout with the **stored** key columns as its input and so derives the prefix differently. For a chained IVM MV whose base tables carry row-id columns of their own (`ivm_use_full_keys`), the base tables' row-id columns land in a different block in the two lists, and every refresh of that MV fails with a spurious error. Stored: k1, k2, __DORIS_IVM_0_ROW_ID_COL__, __DORIS_IVM_1_ROW_ID_COL__, __DORIS_IVM_ROW_ID_COL__, v1, v2 analysed: k1, k2, __DORIS_IVM_ROW_ID_COL__, v1, v2, __DORIS_IVM_0_ROW_ID_COL__, __DORIS_IVM_1_ROW_ID_COL__ Index 3 then compares `__DORIS_IVM_1_ROW_ID_COL__` (largeint) against `v1` (int) and reports "column type not same ... original type is: largeint, current type is: int" -- a name held up against a different column entirely. What the check is for is a base-table change that makes a column disappear or change type, which is a question about names and types rather than about where a column sits, and the refresh maps columns by name. Comparing by name keeps that guard and drops the coupling to a layout the two passes are free to disagree about. Reproduction: `mtmv_p0/ivm/test_ivm_use_full_keys_6`. Its chained MV is built over two IVM MVs; a complete refresh of those replaces their partitions without row binlog, which invalidates the chained MV's baseline, and its own complete refresh then runs this check. The case passes now and fails when this change is reverted. ### Release note None ### Check List (For Author) - Test: Regression test / Unit Test - `mtmv_p0/ivm`: all 97 suites pass, including `test_ivm_use_full_keys_6`, with the stored expectations compared rather than regenerated. - FE unit tests: `MTMVPlanUtilTest`, `MTMVTest`, `MTMVTaskTest`, `IvmBaselineRebuildTest` -- 147 tests, all pass. - Behavior changed: No (the check still rejects a column that disappears or changes type) - Does this need documentation: No
|
run buildall |
|
/review |
|
run buildall |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes on exact head d3f21b0. I found ten independent issues: five P1 correctness/availability or scale problems and five P2 routing, replay, signature, and diagnostic problems.
Checkpoint summary: the normal per-partition epoch path is coherent, but the task/DDL race, commit/publication cut, rename identity, and stream-recreation failover cut break the lifecycle proof. ADD_TASK replay is not equivalent to live state. The name-based schema comparison is safe under existing uniqueness/cardinality invariants, and no new configuration or FE/BE protocol incompatibility was found; the IVM metadata is pre-release. The change also introduces full-cardinality task journaling and inaccurate rebuild/progress observability. Changed tests cover normal invalidation, replay, routing, and successful reconciliation, but miss the reported adversarial interleavings, failover cuts, and scale bound. The focus file supplied no additional focus, so the complete authoritative diff and changed-file call chains were reviewed.
Validation was static only: the authoritative review prompt prohibited builds and tests, so CI/author results were not independently executed. Round 3 found a new independent issue at the configured three-round maximum; convergence is therefore capped/incomplete rather than a clean fixed point.
| } | ||
| // The requirement these partitions are read under, captured before the read inside doRefresh and | ||
| // recorded only if the refresh commits; see captureLatestEpochs. | ||
| Map<String, Long> capturedEpochs = captureLatestEpochs(Sets.newHashSet(needRefreshPartitions)); |
There was a problem hiding this comment.
[P1] Recheck invalidation after choosing the rebuild set. A TRUNCATE can raise this partition's epoch and remove its snapshot after executeIvmAttempt sampled dirtyPartitions. It then enters incrementalScope, this call captures the raised epoch, and the row-delta refresh cannot delete the rows removed only through metadata. Success consequently writes refreshEpoch == latestEpoch and leaves those old MV rows permanently clean. Dirty selection and epoch capture need one generation decision (or an epoch advance here must abort/reroute the partition to rebuild); please add a latch test for the sample -> TRUNCATE -> capture ordering.
| * exists to avoid, so the conservative reading wins. | ||
| */ | ||
| public boolean isDirty() { | ||
| return latestEpoch > refreshEpoch && refreshEpoch != 0; |
There was a problem hiding this comment.
[P1] Do not equate refreshEpoch == 0 with durable emptiness. Alignment journals (0,1) before the first batch, but the MV DML commits before the task publishes its epoch/snapshot and before ADD_TASK is journaled. An FE crash in that cut (or cancel(true) after the executor callback clears executor but before batch publication) leaves committed rows behind replayed (0,1). A later TRUNCATE raises it to (0,2), this returns false, and strict incremental can accept an empty delta and mark the stale rows clean. Persist an in-progress/possibly-populated state before writing, or otherwise make batch commit and durable epoch publication recover as one lifecycle.
| // that failure visible. What the state does to an IVM MV is make the next refresh rebuild the | ||
| // whole MV (MTMVTask#buildAttempts), which a rename back would have it repeat for nothing. A | ||
| // non-IVM MV keeps the state it has always got, which is what its own refresh reads. | ||
| continue; |
There was a problem hiding this comment.
[P1] Preserve the dependency mapping across this rename. BaseTableInfo equality is name-based, but this branch neither invalidates the IVM nor re-keys tableMTMVsOneLevelAndFromView from oldTableInfo to newTableInfo. After t is renamed to tmp, metadata-only DDL such as TRUNCATE on tmp therefore finds no dependent MV; renaming tmp back also looks up only tmp and misses. The query is usable again, yet no dirty epoch exists and the delta stream cannot remove the truncated rows. Please move/alias the dependency entry on rename or keep a conservative invalidation, with a rename -> TRUNCATE -> rename-back regression.
| // the ones partition sync has not created yet, and no per-partition requirement can express that. | ||
| // IVM only -- a non-IVM MV reaches the same effect through its cleared snapshot, which its own | ||
| // refresh already depends on. | ||
| if (mtmv.isIvm() && !request.explicitPartitions |
There was a problem hiding this comment.
[P2] Distinguish whole-IVM invalidation from generic SCHEMA_CHANGE here. processBaseTableChange sets this state even when re-analysis succeeds and no partition epoch was raised (for example DROP COLUMN spare, which the changed test says must not invalidate the baseline). This branch nevertheless converts the following strict INCREMENTAL request into a whole COMPLETE rebuild; the test checks only SUCCESS, so it misses that route. Use a dedicated whole-baseline marker or avoid setting this state for compatible IVM changes, and assert RefreshMode/IvmRebuiltPartitions.
| this.refreshMode = generateRefreshMode(toRebuild); | ||
| executePartitionBasedRefresh(refreshContext, RefreshMode.PARTITIONS, ctx); | ||
| rebuiltSnapshots.putAll(partitionSnapshots); | ||
| recordRebuiltPartitions(request, dirtyPartitions.size()); |
There was a problem hiding this comment.
[P2] Count rebuilt partitions as batches commit. If an early dirty-rebuild group commits and a later group fails, this line is never reached and IvmRebuiltPartitions stays 0 even though ADD_TASK preserves the committed group's epochs/data. The COMPLETE escalation sites have the inverse problem: they record every planned partition before the first batch, so an immediate failure reports all rebuilt. The new diagnostic should reflect successful groups, especially on failed tasks.
| // dropped it, and transparent rewrite reads that map to decide what it may serve. | ||
| Map<String, MTMVRefreshPartitionSnapshot> snapshotsToWrite = partitionSnapshots; | ||
| if (!isReplay && ivmInfo.isEnableIvm()) { | ||
| snapshotsToWrite = snapshotsOfCleanPartitions(partitionSnapshots); |
There was a problem hiding this comment.
[P2] Journal the same filtered snapshot map that is applied live. When an invalidation lands during a task, the leader deliberately omits that dirty partition here, but alterMTMV still contains the raw task map. Replay restores the journaled dirty states, skips this !isReplay filter, and re-adds that snapshot, so restart does not reconstruct the leader's state or preserve the documented removal invariant. With all snapshot-bearing partitions dirty this also changes attempt selection from the no-snapshot COMPLETE branch (rebuilt count 0) to the all-dirty COMPLETE branch (count N). Put a detached snapshotsToWrite on the payload before submitAlterLog and cover invalidation-before-ADD_TASK replay order.
| // here: its batches committed, and without them the partitions it rebuilt would look | ||
| // unsynced and be refreshed again on every following round. | ||
| this.partitionSnapshots.putAll(rebuiltSnapshots); | ||
| this.completedPartitions.addAll(dirtyPartitions); |
There was a problem hiding this comment.
[P2] Keep the progress denominator consistent when merging the rebuild phase. executeSingleIvmAttempt resets needRefreshPartitions to only the incremental scope and clears completedPartitions; this line then adds the dirty rebuilds only to the completed side. A task with one rebuilt and one incremental partition is recorded as 200% (2/1), while a dirty-only success reports null progress because the denominator is empty. Preserve the union of both phase scopes (and their completed sets) for task history.
| LOG.info("IVM MV is in SCHEMA_CHANGE, rebuilding the whole MV, mv={}, taskId={}", | ||
| mtmv.getName(), getTaskId()); | ||
| recordRebuiltPartitions(request, mtmv.getPartitionNames().size()); | ||
| return Lists.newArrayList(RefreshAttemptType.COMPLETE); |
There was a problem hiding this comment.
[P2] Publish the rebuilt plan signature on this direct COMPLETE path. SCHEMA_CHANGE can come from altering a base view while keeping the MV output schema valid but changing its normalized join/layout plan. Because this branch skips the incremental attempt, ivmFallbackReason is never PLAN_SIGNATURE_MISMATCH; executePartitionBasedRefresh discards the signature produced by this successful rebuild and ADD_TASK keeps the old one. The next AUTO refresh then performs a second COMPLETE through mismatch fallback, while a next strict INCREMENTAL rejects the baseline it just rebuilt. Capture/persist the consistent full-refresh signature when this branch establishes the new baseline.
| // this is the point where an entry and the partition it describes become the same thing. | ||
| // Doing it any later would let a partition that sync has just added be refreshed without an | ||
| // entry, and an invalidation arriving in between would have nothing to land on. | ||
| mtmv.alignPartitionStates(mtmv.getPartitionNames()); |
There was a problem hiding this comment.
[P1] Avoid making every later ADD_TASK carry this full aligned map. The existing task-result path deep-copies partitionStates under mvRwLock and serializes the whole map as JSON, but before this change the production map was never populated. This line now creates one entry per MV partition, so even a no-op scheduled refresh or a task advancing one partition emits O(total partitions) state. Doris already exercises 160,000 mapped MV partitions, making each periodic result a multi-megabyte record and lock-held copy. Persist only the task's detached epoch delta on ADD_TASK (and omit it when empty), keeping full maps for alignment/invalidation records.
| // The barrier goes first: a stream this rebuild reconciles carries the base table's current | ||
| // rows as its initial snapshot, and a later incremental refresh that consumed it as a delta | ||
| // against data still built from the old baseline would double-count them. | ||
| writeIvmBaselineBarrier(RefreshMode.COMPLETE); |
There was a problem hiding this comment.
[P1] Keep a durable rebuild requirement before recreating an IVM stream. A dropped/unusable stream sends a fallback refresh to COMPLETE, but reconcileIvmStreams durably creates its replacement with show_initial_rows=true before any MV rebuild batch. If FE crashes after that create and before the rebuild/ADD_TASK, restart has the old populated MV with clean nonzero epochs plus a usable stream whose historical rows are exposed as APPEND; the next AUTO/INCREMENTAL refresh can therefore add those rows to the old baseline again. This removed barrier protected exactly that cut. Persist a whole-MV/affected-partition requirement before reconciliation and clear it only after the rebuilt baseline is durably published, with a failover test at this boundary.
### What problem does this PR solve? Issue Number: N/A Related PR: apache#68390 Problem Summary: `MTMVRelationManager` keys its dependency maps by `BaseTableInfo`, which compares by name, and a rename was handled by putting the dependent MVs into `SCHEMA_CHANGE` instead of keeping the lookup working. That left a hole as soon as the rename stopped setting that state for an IVM MV: a rename leaves the MV query spelling the old name, so the query no longer analyses and the MV's relation is never recomputed -- the maps keep the old name. A metadata-only change to the table under its new name, a TRUNCATE for instance, then finds no dependent MV to invalidate, and renaming the table back restores an analyzable query whose MV still holds the rows that change removed, with nothing naming the partition to rebuild. `alterTable` now moves the renamed table's entries in `tableMTMVs` and `tableMTMVsOneLevelAndFromView` to the new name, registering them under the new name before dropping the old one, so a concurrent base-table change either still finds the old name or already finds the new one. The invalidation itself runs first, while the dependencies are still registered under the name being left: the lookup is by the old name, so moving the entries first would make it find nothing and the rename would stop invalidating anything at all -- for a non-IVM MV as much as for an IVM one, which is the behaviour that has to stay as it was. ### Release note None ### Check List (For Author) - Test: Unit Test - `IvmBaselineRebuildTest` (38 tests) and the full set run for this branch -- `MTMVPlanUtilTest, MTMVTest, MTMVTaskTest, IvmBaselineRebuildTest, AlterMTMVTest, IvmInfoTest, MTMVRefreshSnapshotTest` -- 185 tests, all pass. The ordering above is what `IvmBaselineRebuildTest#testRenameStillInvalidatesANonIvmMv` pins: it fails when the entries are moved before the invalidation and passes with this order. - Behavior changed: No (a non-IVM MV keeps the state a rename has always given it; an IVM MV keeps the state this PR's earlier commit stopped giving it, and gains a lookup that keeps working) - Does this need documentation: No
…an be interrupted at ### What problem does this PR solve? Issue Number: N/A Related PR: apache#68390 Problem Summary: A code review of this PR found five correctness problems in how the per-partition requirement the refresh routes on is read, published and recovered. They share the state contract, so they are landed together; each is listed with the review item it answers. * P1-2: `refreshEpoch == 0` was read as "the partition holds no rows". A refresh commits the MV data transaction before its task result is journaled, so a crash in between leaves rows behind a pair the MV loaded as "never refreshed"; a later invalidation raising `latestEpoch` then found nothing dirty and a strict INCREMENTAL kept the rows. `MTMVPartitionState` gains an `inProgress` flag, written before the data transaction, and `needsRebuild()` -- what the routing reads -- is wider than `isDirty()` by it. A payload written before the member existed reads as "not in progress". * P1-9: a task result journaled the whole aligned map, so every periodic refresh emitted one entry per MV partition -- an O(partitions) record and a lock-held deep copy, on 160,000-partition MVs. It now journals only the partitions it published, and the ADD_TASK replay merges per entry instead of assigning, because the entries it omits belong to other records. The state-map channel (ALTER_PARTITION_STATES) still replaces: that one carries the whole map. * P1-1: dirty selection and epoch capture were two reads. A TRUNCATE landing between them was captured by the incremental attempt, which then published an epoch that said the rows it could not remove were current. The plan now reads the states once and carries a per-partition ceiling, and the publish clamps to `min(captured, planned)`: a mark that lands mid-refresh leaves the partition dirty for the next refresh instead of being swallowed. * P1-10: `reconcileIvmStreams` durably creates a replacement stream (its historical rows exposed as APPEND) before any rebuild batch. A crash in between leaves a populated MV with clean epochs and a usable stream, and the next refresh adds those rows to the old baseline again. The partitions are marked before reconciliation now -- the durable requirement the removed barrier used to write. * P2-5 / P2-7: the rebuilt-partition count was recorded when the rebuild was planned, so a task that failed part way claimed partitions it never replaced; it is recorded from the committed batches instead, and the progress denominator is the union of the rebuild and incremental phases rather than the incremental scope alone. Also covers P2-5 and P2-7, which are diagnostic only. ### Release note None ### Check List (For Author) - Test: Unit Test / Regression test - FE unit tests: `MTMVPlanUtilTest`, `MTMVTest`, `MTMVTaskTest`, `IvmBaselineRebuildTest`, `AlterMTMVTest`, `IvmInfoTest`, `MTMVRefreshSnapshotTest` -- 185 tests, all pass. - Regression: `mtmv_p0/ivm` -- all 97 suites pass with the stored expectations compared. - Positive controls: the in-progress criterion, the delta payload and the progress union each fail their case when reverted. - Behavior changed: No (a partition being published is rebuilt rather than skipped, which is what the removed barrier asked for as well) - Does this need documentation: No
…ll causes ### What problem does this PR solve? Issue Number: N/A Related PR: apache#68390 Problem Summary: Dropping a column the MV does not read leaves the IVM baseline alone, but the shared base-table change hook still moves the MV into SCHEMA_CHANGE, and this PR reads that state as "the whole MV must be rebuilt". The case asserted only that the refresh succeeded, so the escalation it was actually getting went unnoticed; it now pins RefreshMode and IvmRebuiltPartitions as well. Narrowing the hook so a change that re-analyses cleanly leaves an IVM MV alone is a separate change; the assertion is here so the escalation cannot change again unnoticed until then. ### Release note None ### Check List (For Author) - Test: Regression test - `mtmv_p0/ivm/test_ivm_drop_referenced_column_baseline_rebuild` passes with the stored expectations compared, not regenerated. - Behavior changed: No (test only) - Does this need documentation: No
|
P2-8 (publish the rebuilt plan signature on the direct COMPLETE path) — not taken, and here is the evidence. I implemented it: widening
Bisected and isolated: reverting every other change from this round keeps them red, and restoring everything while reverting only this one makes them green again. The mechanism is the one the finding proposes, read the other way round: publishing the fresh signature lets the next refresh take the incremental path instead of the Fixing what the finding is really after needs the incremental path's offset/baseline handling understood first, so I have left the condition as it was rather than widen it. Recording it here so it is not lost. |
|
run buildall |
|
/review |
There was a problem hiding this comment.
Requesting changes on exact head acad27a. I found five new distinct issues: three P1 rename-lifecycle correctness failures and two P2 lock/fallback efficiency problems.
Checkpoint summary: the per-partition epoch and replay paths are otherwise conservative, and previously reported direct-IVM durability, generic SCHEMA_CHANGE, signature, diagnostics, stream, snapshot, and scale issues were treated as duplicate fences rather than repeated. The new rename key move is not durable across replay/image reconstruction, can be undone by stale task publication, and is not atomic with catalog visibility. The name/type schema contract, chained hidden-column binding, replace/drop paths, compatibility, and error handling produced no additional distinct finding. The changed tests cover normal sequential rename and rebuild behavior but miss replay, stale-publication, catalog-to-hook interleavings, delayed journal completion, and rebuild-then-fallback execution count. No user review focus was specified, so the complete authoritative diff and all 31 changed files were reviewed.
Validation was static only: the authoritative review instructions prohibited builds and tests, so author/CI results were not independently executed. Three review rounds converged with no additional valuable findings in the final round.
| * carry: a rename changes no rows, so there is nothing to rebuild, only a lookup that has to keep | ||
| * working. | ||
| */ | ||
| private void renameBaseTable(BaseTableInfo oldTableInfo, BaseTableInfo newTableInfo) { |
There was a problem hiding this comment.
[P1] Make this dependency re-key survive replay and cache reconstruction. The move only changes these derived maps; every dependent MTMV still persists the old name in MTMV.relation, registerMTMV rebuilds the maps from that relation, and replayRenameTable never calls this hook. After rename t -> tmp and a restart/replay, TRUNCATE tmp therefore finds no dependent; renaming tmp back leaves the old MV rows with no dirty epoch. Persist/replay the relation rename (or use stable table identity), and cover rename -> restart/replay -> TRUNCATE -> rename-back.
| // Registered under the new name before the old one is dropped: a concurrent base-table change | ||
| // either still finds the old name or already finds the new one, never neither. Merged rather than | ||
| // replaced, because a table dropped and re-created under this name registers its own dependents. | ||
| map.computeIfAbsent(newTableInfo, key -> Sets.newConcurrentHashSet()).addAll(dependents); |
There was a problem hiding this comment.
[P1] Fence refresh relations captured before this rename. A task can finish its DML with relation keyed by t, then t -> tmp moves the live key here without advancing the IVM schema generation. The later accepted ADD_TASK calls refreshComplete with that stale relation; refreshMTMVCache re-adds t and prunes tmp. TRUNCATE tmp is then invisible, and renaming back exposes stale rows. Reject/translate pre-rename task relations with a dependency generation, and add a latch test for task return -> rename -> result publication -> TRUNCATE.
| // Every partition has to be rebuilt, including the ones this MV does not have yet, so the | ||
| // MV goes into the state that says exactly that. Journaled on its own record, ahead of the | ||
| // property change below; a replay applies both in that order. | ||
| invalidateWholeMv("The MV's refresh baseline changed with its properties"); |
There was a problem hiding this comment.
[P2] Do not wait for this status journal while holding mvRwLock. invalidateWholeMv re-enters alterStatus and then processAlterMTMV calls synchronous logAlterMTMV; batch mode waits for the edit-log worker and direct mode performs the write here. A slow journal therefore blocks every reader, invalidation, and task-result publication for this MV, contrary to the submit-under-lock/await-after-unlock pattern used by the surrounding property record. Enqueue the ordered status and property records under the lock, then await both after releasing it.
| this.completedPartitions.addAll(rebuildCompleted); | ||
| return AttemptResultType.SUCCESS; | ||
| } | ||
| if (ivmResult.getFailureReason() != IvmFailureReason.MV_PARTITION_NOT_FOUND) { |
There was a problem hiding this comment.
[P2] Preserve the dirty rebuild when IVM falls back to PARTITIONS. The rebuild above commits and saves its snapshots only in local rebuiltSnapshots, then executeSingleIvmAttempt resets the task accumulators. On any fallback-allowed result this return happens before those locals are merged, so the following PARTITIONS attempt replans against mtmv's still-missing invalidated snapshot and INSERT OVERWRITEs every just-rebuilt partition again. Carry the committed rebuilt set/snapshots into fallback planning or exclude that set, and add a forced-fallback test that asserts each dirty partition is rebuilt once.
| if (CollectionUtils.isEmpty(dependents)) { | ||
| return; | ||
| } | ||
| // Registered under the new name before the old one is dropped: a concurrent base-table change |
There was a problem hiding this comment.
[P1] Close the gaps in this rename transition. Env.renameTable exposes and journals tmp, then releases the database/table locks before Alter later invokes this hook, so TRUNCATE tmp or DROP PARTITION can mark while the map is still keyed only by t and commit with no rebuild epoch. Even after this method starts, computeIfAbsent publishes an empty tmp set before addAll fills it. This rename then skips IVM invalidation and only moves the key. Make catalog visibility and the populated dependency entry atomic (or conservatively invalidate across both gaps), and latch-test rename -> metadata DDL -> hook -> rename-back.
2 participants
What problem does this PR solve?
Issue Number: N/A
Related PR: #68170, #68180, #68193
Trace issue: #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 and nothing incremental can remove those rows. Today the invalidation is recorded at MV granularity:IvmInfo.completeBaselineRebuildRequired/pendingBaselineRebuildPartitionsplus aschemaChangeVersionguard. That granularity is coarse -- one dirty partition drags the whole MV to a COMPLETE refresh, a task result produced before the invalidation is discarded, and a strictREFRESH ... INCREMENTALis rejected until a COMPLETE refresh has run, even when the change touched nothing the MV reads.This PR replaces the barrier with a per-MV-partition requirement:
MTMV.partitionStatesmaps each MV partition to{refreshEpoch, latestEpoch}(persisted aspst, journaled throughALTER_PARTITION_STATES). A partition is dirty ifflatestEpoch > refreshEpoch && refreshEpoch != 0--refreshEpoch == 0means it was never refreshed, so it holds no rows and its first refresh reads the current base tables anyway.refreshEpochonce that batch's data is committed, so an invalidation arriving mid-refresh is not swallowed.SCHEMA_CHANGE, the refresh runs as COMPLETE. A refresh that rebuilt partitions the request did not ask for reports how many in the newIvmRebuiltPartitionscolumn of the mv task TVF.Behaviour changed: Yes
REFRESH ... INCREMENTALthat meets an invalidated baseline no longer fails with "IVM baseline rebuild is pending"; it rebuilds (the whole MV for a schema-level invalidation, the invalidated partitions otherwise) and reports the count inIvmRebuiltPartitions.SCHEMA_CHANGErefreshes as COMPLETE without first attempting the incremental rewrite, soIvmFallbackReasonstays unset where it previously reported the barrier's label.NORMAL.Release note
None
Check List (For Author)
IvmBaselineRebuildTest,MTMVTaskTest,MTMVTest,MTMVPlanUtilTest,CreateMTMVCommandTest,MTMVRelationManagerTest,AlterMTMVTest,IvmInfoTest,MTMVRefreshSnapshotTest,MTMVPartitionUtilTest,MetaLockUtilsTest-- 147 tests re-run for the last commit, all pass. Every new case was checked to fail when the change it covers is reverted, including the schema comparison above.mtmv_p0/ivm-- all 97 suites pass with the stored expectations compared, not regenerated. That includestest_ivm_use_full_keys_6, which is the case that reproduces the chained-MV schema comparison, andtest_ivm_partition_epoch_rebuild(new),test_ivm_baseline_marker_scope,test_ivm_partition_baseline_rebuild,test_ivm_partition_baseline_rebuild_dup_keys,test_ivm_partition_sync_limit,test_ivm_chained_mtmv_1,test_ivm_chained_mtmv_2,test_ivm_partition_drop_live_delta,test_ivm_partition_window_remove,test_ivm_drop_referenced_column_baseline_rebuildandtest_ivm_strict_incremental_rebuilds_invalidated_partitions(renamed fromtest_ivm_strict_failure_partition_atomicity), whose expectations this PR updates.