yujun777 commented on code in PR #68180:
URL: https://github.com/apache/doris/pull/68180#discussion_r4068973790
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/MTMV.java:
##########
@@ -682,33 +687,238 @@ public void invalidateIvmBaseline() {
editLogItem.await();
}
- public void invalidateIvmBaseline(BaseTableInfo baseTableInfo, Map<String,
Long> changedPartitions) {
+ /**
+ * Mark the MV partitions that may hold rows read from the changed base
table partitions as needing a
+ * rebuild. When those partitions cannot be determined, the whole MV is
marked instead.
+ */
+ /**
+ * @return whether a barrier was recorded. The caller reports the two
outcomes differently: a change
+ * that no MV partition reads leaves nothing to rebuild and must
not be logged as one.
+ */
+ public boolean invalidateIvmBaseline(BaseTableInfo baseTableInfo,
Map<String, Long> changedPartitions) {
+ // Computed before the MV lock is taken, not inside it: the mapping
reads the partition items of the
+ // MV and of every PCT table, so it takes those tables' locks, and the
MV lock has to stay a leaf
+ // (nothing may be acquired under it) the way the rest of this class
assumes. The selection does not
+ // need to be atomic with the barrier it produces: the barrier is
recorded under the lock below, and
+ // the names it carries are intersected with the live partition names
when they are consumed
+ // (MTMVTask).
+ Optional<Set<String>> affectedMvPartitions =
selectAffectedMvPartitions(baseTableInfo,
+ changedPartitions);
+ if (affectedMvPartitions.isPresent() &&
affectedMvPartitions.get().isEmpty()) {
+ // No MV partition reads any of the changed base partitions, so
this change cannot leave
+ // anything behind here: there is no barrier to persist, and
skipping the version bump
+ // keeps it from discarding the result of a task that is already
running.
+ LOG.debug("No MV partition is affected by changed base partitions,
mv={}, baseTable={}, "
+ + "changedPartitions={}", name, baseTableInfo,
changedPartitions);
+ return false;
+ }
EditLogItem editLogItem;
writeMvLock();
try {
if (ivmInfo == null) {
ivmInfo = new IvmInfo();
}
- if (mvPartitionInfo.getPartitionType() !=
MTMVPartitionType.SELF_MANAGE
- && mvPartitionInfo.getPctInfos().stream()
- .anyMatch(pctInfo ->
pctInfo.getTableInfo().equals(baseTableInfo))) {
- Optional<Set<String>> mvPartitionNames =
refreshSnapshot.getMvPartitionNames(baseTableInfo,
- changedPartitions);
- if (mvPartitionNames.isPresent()) {
-
ivmInfo.addPendingBaselineRebuildPartitions(mvPartitionNames.get());
- } else {
- // Without a snapshot for every changed base partition, a
PARTITIONS rebuild is unsafe.
- ivmInfo.requireCompleteBaselineRebuild();
- }
- } else {
+ if (!affectedMvPartitions.isPresent()) {
+ // A narrower rebuild could leave a partition holding rows of
the changed base partition
+ // untouched, and those rows cannot be repaired later: the
change emitted no row binlog.
ivmInfo.requireCompleteBaselineRebuild();
+ } else {
+
ivmInfo.addPendingBaselineRebuildPartitions(affectedMvPartitions.get());
}
schemaChangeVersion++;
editLogItem = submitIvmInfoChange();
} finally {
writeMvUnlock();
}
editLogItem.await();
+ return true;
+ }
Review Comment:
Fixed in 9214167dba2, and your reading of the two reads is right: they do
not bracket a window that is set and cleared inside the construction. The read
before it happens before the window exists and the read after it after the
window is gone, so a mapping built in between is filtered while both answer "no
limit". My previous reply claimed the bracket covered this; it covers the
window being *cleared* during the construction and the window being *set*
during it, not both in one interval.
The fix takes the second route you offered, in the form this class already
uses for the two properties that make the same trade. A `partition_sync_limit`
window decides which base partitions the MV maintains, so changing it changes
which rows the MV owes -- the reasoning that already makes an
`excluded_trigger_tables` change and an `ivm_partition_window_limit`
enlargement force a complete baseline rebuild in `alterMvProperties`. A change
to any of the window properties now does the same, compared before and after
inside the same MV write lock that applies the ALTER, so the ALTER itself
records the barrier:
```java
boolean containsSyncWindow =
MTMVPropertyUtil.containsPartitionSyncWindow(mvProperties);
Map<String, String> oldSyncWindow = containsSyncWindow
? MTMVPropertyUtil.partitionSyncWindowOf(this.mvProperties) : null;
this.mvProperties.putAll(mvProperties);
...
if (containsSyncWindow && ivmInfo != null && ivmInfo.isEnableIvm()
&&
!MTMVPropertyUtil.partitionSyncWindowOf(this.mvProperties).equals(oldSyncWindow))
{
requireCompleteBaselineRebuild = true;
}
```
Restating the same window compares equal and forces nothing, so an ALTER
that changes no window does not cost a rebuild. The two reads in the selector
stay, because they answer a different half: a window that is *set* and stays
set is seen by the read taken after the mapping, and the read before it covers
a window that was already in effect when the selection started.
The three orderings are therefore covered, and I do not think a fourth
exists: a mapping built under a window that both reads answer "no limit" for
requires an active-to-inactive change inside that interval, which is exactly
what now forces the rebuild. A window that goes inactive before the selection
starts leaves the mapping complete, which is the sound case.
Coverage:
`IvmBaselineRebuildTest.testChangingTheSyncWindowRequiresCompleteBaselineRebuild`
-- setting the window requires a complete rebuild, restating the same window
does not, widening it does; it fails without the rule with `expected: <true>
but was: <false>`. One consequence worth stating rather than leaving to be
found: the three tests that set the window now record that barrier themselves,
so they release it before the change they are actually testing. Without that,
their assertions would hold because of the ALTER rather than because of the
selection.
Verification on this head: `IvmBaselineRebuildTest` 37/37,
`MTMVPropertyUtilTest` 13/13, `MTMVTest` 23/23, `checkstyle:check` clean, and
`mtmv_p0/ivm` test_ivm_partition_sync_limit,
test_ivm_partition_sync_limit_with_window, test_ivm_baseline_marker_scope,
test_ivm_partition_baseline_rebuild,
test_ivm_partition_baseline_rebuild_dup_keys,
test_ivm_drop_referenced_column_baseline_rebuild,
test_ivm_partition_drop_live_delta, test_ivm_replace_table_reconcile,
test_ivm_replace_stream_cleanup and test_ivm_drop_column_fallback_reason green
on a local cluster built from it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
