You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@doris.apache.org by "yujun777 (via GitHub)" <gi...@apache.org> on 2026/10/08 03:35:48 UTC
[I] [Bug] A light base table schema change is visible to the query planner before the materialized views that read it are invalidated [doris]
yujun777 opened a new issue, #68767:
URL: https://github.com/apache/doris/issues/68767
### Version
master (and any branch carrying the MTMV query judgement, #68646). Found while working on the IVM series tracked by #65418.
### What's Wrong?
A light base table schema change becomes visible to the query planner **before** the materialized views that read the table are invalidated, and nothing in the rewrite's eligibility check notices that the two are out of step.
For a light schema change the new schema is installed inside the base table's write lock, and the MTMV hook that puts the views reading the table into `SCHEMA_CHANGE` runs only after that lock has been released:
- `SchemaChangeHandler.process` holds `olapTable.writeLockOrDdlException()` for its whole body, calls `modifyTableLightSchemaChange(...)` (which installs the new column) and releases the lock at the end of the method -- `fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java:2677-2701`.
- Back in the caller, the hook runs after that lock is gone -- `fe/fe-core/src/main/java/org/apache/doris/alter/Alter.java:376-400` (`Env.getCurrentEnv().getMtmvService().alterTable(...)`).
In that window a query can be planned against the new schema and transparently rewritten to a materialized view that is still `NORMAL`, whose rows were computed under the pre-change binding of the name it moved. A light schema change does not bump the base table's visible version, and the view's snapshot is unchanged, so `SyncWithBaseTables` stays true and the rewrite's checks pass.
Nothing in the hook itself is wrong here; the deficit is that between "the schema is visible" and "the views that read it have been judged" there is an interval in which the rewrite still trusts the views.
Two things widen or move the window:
- The query judgement added in #68646 re-analyses each dependent view's query inside the hook before deciding, so the interval grows from "one state change per dependent view" to "one query analysis per dependent view". On master the same window exists, just narrower.
- Observers have the same interval through the journal, and it is not bounded by an analysis: the light schema change is journaled first and the view invalidation as its own record after it, so a replayed (already published) schema is visible to queries on an observer before the invalidation record has been applied there.
### What You Expected?
The transparent rewrite must not serve a materialized view in the interval between a base table schema change being published and the views that read it being invalidated -- either the invalidation is established before the new schema becomes visible, or the rewrite refuses those views until they have been judged against the change.
### How to Reproduce?
The window is an interval between two statements of one DDL, so it is not a deterministic reproduction; it needs a planner running concurrently with a light DDL. The shape it bites in is a light change that makes a view's query bind a name to another column, e.g.:
```sql
-- t1(k1 int, v1 int) with light schema change enabled
-- mv1: SELECT k1, SUM(v1) AS total FROM t1 GROUP BY k1 WHERE flag IN (SELECT flag FROM t2)
-- (a name the stored query reaches unqualified, so a column added to t1 answers for it from then on)
ALTER TABLE t1 ADD COLUMN flag INT DEFAULT 0; -- light: published under the write lock, invalidated after
```
A query planned in the window binds `flag` to the new column and can be rewritten to `mv1`, whose rows were computed while `flag` was answered for by another column.
What is verified today is the code path above (the publish and the invalidation are in two different lock scopes); the wrong result itself needs a concurrent planner inside the window and has not been reproduced deterministically.
### Anything Else?
Options considered, with their costs:
- **(A) A judgement barrier the rewrite respects** -- a transient marker on the views that read the table, set before the new schema is published and cleared once the views have been judged. It has to be a count rather than a flag (two concurrent ALTERs on the same table would clear each other's window) and it must be cleared in a `finally` (a leaked mark silently keeps a view out of the rewrite forever). It closes only the leader's window; the observer interval above needs the marker to be replayable or a different criterion.
- **(B) Decide under the write lock, journal after** -- move the judgement into the lock scope that publishes the schema, and defer only the edit log waits (holding a table write lock across a journal wait is against the locking rules, and the query analysis would then hold the table write lock for every dependent view).
- **(C) Compare a schema identity** -- have a view record the base table's schema identity when it refreshes, and have the rewrite require it to still match. This is the only one of the three that also closes the observer interval, and it is the largest: a new persisted field written by every refresh, plus replay and image compatibility.
Related: #68646 (the judgement that widened the window), #65418 (the IVM tracker).
--
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: commits-unsubscribe@doris.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [I] [Bug] A light base table schema change is visible to the query planner before the materialized views that read it are invalidated [doris]
Posted by "yujun777 (via GitHub)" <gi...@apache.org>.
yujun777 commented on issue #68767:
URL: https://github.com/apache/doris/issues/68767#issuecomment-6060338828
Thanks -- the correction is in the body now, and it splits the two things that were conflated there:
- master does not reach the hook for an add at all (`AddColumnOp#needChangeMTMVState` and `AddColumnsOp#needChangeMTMVState` return false on master), so an add invalidates nothing there, and the interval for an add arrives with #68646 rather than being widened by it;
- for the operations that do reach the hook, the interval pre-exists and is the publication order itself: the schema is installed inside the table's write lock and the hook runs after it is released. What #68646 changes is its width -- from "one state change per dependent view" to "one query analysis per dependent view".
On the reproduction: you are right that the snippet was schematic and not valid SQL, and that the wrong result itself is not demonstrated here. What is verified is the interval (the two lock scopes above), and that a name can move at all: the same movement, in a shape that does not need a concurrent planner, is demonstrated on #68646's `QUALIFY` thread -- a view created before that fix keeps `... QUALIFY flag = 1` in the catalog, and after `DROP COLUMN flag` it stays NORMAL and in sync holding no rows while the same query returns `(1,1)`. What this interval adds is that the movement can be observed by a planner while the view is still NORMAL: the view is not yet wrong for the query it was built for, it is stale for the query the new schema makes of it.
A fixture for the deterministic test, which the interval needs and which the scope-add regression does not cover:
```sql
CREATE TABLE t1 (id INT, flag INT) DUPLICATE KEY(id) DISTRIBUTED BY HASH(id) BUCKETS 1
PROPERTIES ("replication_num" = "1");
CREATE TABLE t2 (id INT) DUPLICATE KEY(id) DISTRIBUTED BY HASH(id) BUCKETS 1
PROPERTIES ("replication_num" = "1");
INSERT INTO t1 VALUES (1, 1);
INSERT INTO t2 VALUES (1);
CREATE MATERIALIZED VIEW mv BUILD IMMEDIATE REFRESH COMPLETE ON MANUAL
DISTRIBUTED BY HASH(id) BUCKETS 1 PROPERTIES ("replication_num" = "1")
AS SELECT o.id FROM t1 o WHERE EXISTS (SELECT 1 FROM t2 i WHERE i.id = o.id AND flag = 1);
ALTER TABLE t2 ADD COLUMN flag INT DEFAULT 0; -- light: installed under the write lock
```
`flag` inside the subquery is the outer table's before the add and `t2`'s after it, so a query planned between the unlock and the invalidation binds the new column and can be answered from a view whose rows were computed with the outer one. The latch would sit between those two points -- after `modifyTableLightSchemaChange` and before `MTMVService#alterTable` -- and the plan built there is the interesting one with a cold cache (`mtmv_cache_manage_num = 0`); the same SELECT with `enable_materialized_view_rewrite = false` is the control for the rows.
On the mechanism notes: the existing index schema version is a good input for the third option -- a view records the identity of the schema it refreshed against and the rewrite requires it to still match -- since `updateBaseIndexSchema` bumps it for a light change while leaving the data visible versions alone, which is exactly the version that has to be compared. And the observer side is why the first option (a transient barrier) is not enough on its own: the interval there is a journal order rather than a lock order, so the barrier would have to be replayable, or replaced by a comparison.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [I] [Bug] A light base table schema change is visible to the query planner before the materialized views that read it are invalidated [doris]
Posted by "Doris-Breakwater (via GitHub)" <gi...@apache.org>.
Doris-Breakwater commented on issue #68767:
URL: https://github.com/apache/doris/issues/68767#issuecomment-6051754595
Breakwater-GitHub-Analysis-Slot: slot_2c2a6eb07530
The schema-publication / MTMV-invalidation interval is supported by the source. I recommend prioritizing this as a query-correctness investigation, with an important distinction: the interval and missing schema check are verified statically; an actual stale rewrite and wrong result still need a deterministic concurrent test.
I inspected public master at `81556a1b5ef5d52412a8380e505238bc744f46a8` and the current head of [PR #68646](https://github.com/apache/doris/pull/68646), `45a1894dfe406f611d1cb00b1e00daea38ed2681`. The PR is currently open and unmerged. This issue is open with no labels. No Doris code was changed, and no build or runtime test was run.
The relevant evidence is:
- On master, `SchemaChangeHandler.process` acquires the base-table write lock, installs the light change, and releases the lock before returning. `Alter` calls the MTMV hook afterward. `updateBaseIndexSchema` increments the index schema version, but does not advance the table/partition data visible versions. See [schema publication and unlock](https://github.com/apache/doris/blob/81556a1b5ef5d52412a8380e505238bc744f46a8/fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java#L2677-L2704), [schema version update](https://github.com/apache/doris/blob/81556a1b5ef5d52412a8380e505238bc744f46a8/fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java#L3734-L3765), and [the later hook](https://github.com/apache/doris/blob/81556a1b5ef5d52412a8380e505238bc744f46a8/fe/fe-core/src/main/java/org/apache/doris/alter/Alter.java#L375-L378).
- Rewrite checks MV state and partition freshness. OLAP freshness snapshots compare visible versions and table/partition IDs, without the index schema version. Thus a previously fresh MV can still pass these gates during this interval. The planner's table read locks stabilize the schema it observes, but do not couple schema publication to the later MV state transition. See [rewrite eligibility](https://github.com/apache/doris/blob/81556a1b5ef5d52412a8380e505238bc744f46a8/fe/fe-core/src/main/java/org/apache/doris/mtmv/MTMVRewriteUtil.java#L52-L126), [OLAP snapshots](https://github.com/apache/doris/blob/81556a1b5ef5d52412a8380e505238bc744f46a8/fe/fe-core/src/main/java/org/apache/doris/catalog/OlapTable.java#L3867-L3895), and [planner locking](https://github.com/apache/doris/blob/81556a1b5ef5d52412a8380e505238bc744f46a8/fe/fe-core/src/main/java/org/apache/doris/nereids/StatementContext.java#L1069-L1096).
- A cache miss matters: `getOrGenerateCache` can analyze the stored MV SQL against the newly visible schema while the MV still contains pre-change rows. Its cache-generation guard is not a comparison against the base table's schema identity. A warm pre-change plan might instead fail rewrite matching, so eligibility alone does not prove that every query returns incorrect rows. See [cache construction](https://github.com/apache/doris/blob/81556a1b5ef5d52412a8380e505238bc744f46a8/fe/fe-core/src/main/java/org/apache/doris/catalog/MTMV.java#L633-L693).
- Replay publishes the light schema under the table lock without invalidating dependent MVs in that operation. MV invalidation arrives separately through `OP_ALTER_MTMV` / `ALTER_STATUS`. `Env.replayJournal` processes records individually; an already readable observer is not made unreadable for every replay batch. See [light-schema replay](https://github.com/apache/doris/blob/81556a1b5ef5d52412a8380e505238bc744f46a8/fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java#L3625-L3649) and [replay/readability handling](https://github.com/apache/doris/blob/81556a1b5ef5d52412a8380e505238bc744f46a8/fe/fe-core/src/main/java/org/apache/doris/catalog/Env.java#L3182-L3263). This supports an observer interval as well.
One version-specific correction: on the inspected master, `AddColumnOp.needChangeMTMVState()` and `AddColumnsOp.needChangeMTMVState()` both return false. For the issue's ADD example, master therefore skips the hook entirely; that case is not merely a narrower temporary interval. The PR changes ADD to invoke the hook. For hook-triggering operations, master already re-analyzes each dependent MV before its unconditional invalidation. Please separate the master ADD coverage gap from the publication-order race. See [master ADD behavior](https://github.com/apache/doris/blob/81556a1b5ef5d52412a8380e505238bc744f46a8/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/AddColumnOp.java#L120-L123) and [master per-MV analysis/invalidation](https://github.com/apache/doris/blob/81556a1b5ef5d52412a8380e505238bc744f46a8/fe/fe-core/src/main/java/org/apache/doris/mtmv/MTMVRelationManager.java#L491-L521).
To establish the user-visible failure, please provide the exact tested FE commit, table and MV DDL, inserted rows, the persisted MV SQL, the SELECT being rewritten, FE role/topology, and rewrite/cache settings. The sample is schematic: `WHERE` must precede `GROUP BY`, and the pre-change binding of `flag` needs a valid scope. An unqualified reference inside a correlated subquery is a better fixture; [the PR's scope-add regression](https://github.com/apache/doris/blob/45a1894dfe406f611d1cb00b1e00daea38ed2681/regression-test/suites/mtmv_p0/test_drop_unreferenced_column_mtmv.groovy#L178-L206) provides that shape, but does not test the concurrent interval.
Suggested next steps:
1. Add a latch-controlled test paused after schema publication/unlock and before the hook. Use a successfully refreshed MV, a cold plan cache (also test supported `mtmv_cache_manage_num=0`), and rows whose result changes with the binding. Capture the chosen plan and compare results with `enable_materialized_view_rewrite=false`. Retain MV state, `SyncWithBaseTables`, schema/data versions, and timestamped FE events. Avoid relying on repeated ALTER/SELECT timing alone.
2. Test a readable observer paused after replaying the schema record and before the MV invalidation record. Record both journal IDs. Include overlapping ALTERs, DDL failure, concurrent refresh completion, restart, and failover in the fix's coverage.
3. Choose a mechanism that also covers replay. A transient barrier needs reference counting across overlapping changes, exception-safe release, and a replay/persistence design. Moving judgement under the leader's write lock alone leaves the observer interval; it also needs consistent multi-table lock ordering and journal waits outside metadata locks. An atomic schema-plus-invalidation replay transition is another option, so schema identity is not the only possible distributed solution.
4. If using schema identity, evaluate the existing base-index ID/schema version as inputs rather than inventing an unrelated counter. Capture the identity of the schema actually used by refresh and validate it before publishing the result. Check it before grace-period or relaxed-consistency shortcuts, handle missing identities conservatively, and cover image/journal compatibility. A whole-schema comparison also rejects otherwise harmless column changes until refresh or explicit revalidation, which should be an intentional tradeoff.
Until a fix is validated, disabling transparent MV rewrite for affected sessions (`SET enable_materialized_view_rewrite = false`) is a source-supported way to avoid this rewrite path. This does not establish that a stored MV's rows are current.
--
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: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org