Refuse an insert into a materialized view whose target is a view - #115985
Conversation
Inserting into a materialized view declared `TO <another materialized view>` aborted the
server in debug and sanitizer builds:
std::exception. Code: 1001, type: std::out_of_range,
e.what() = unordered_map::at: key not found
DB::InsertDependenciesBuilder::createPreSink(DB::StorageIDMaybeEmpty) const
`observePath` rejects a hop whose view does not select from its parent. That check is for a
stale dependency left behind by an ALTER, and before the pushing-to-views rework it only ran
while iterating dependent views. It is now also reached from the target hop, where a view
never selects from the view forwarding into it, so the check always rejects: the path is
abandoned, no output header is recorded for the view, and `createPreSink` then calls
`output_headers.at(view_id)` on a missing key. Guarding that `.at()` alone would not help,
because the hop has no sink either - `createSinkImpl` reaches `UNREACHABLE()` for a
materialized view inner storage.
Refuse the insert with NOT_IMPLEMENTED instead, in the shape `createRetry` already uses for
a forwarding path it cannot rebuild. Reachable from `INSERT ... VALUES`, from
`INSERT ... SELECT` through `addInsertToSelectPipeline`, and from `CREATE ... POPULATE`
through `fillTableIfNeeded`; the AST fuzzer found it on the third.
The refusal is limited to the view the insert addresses. A dependent view whose target name
now resolves to a view is still skipped, which is what the stale-dependency check is for and
what 04216_materialized_view_reused_target_name_dependency asserts.
|
Workflow [PR], commit [55f2479] Summary: ✅
AI ReviewSummaryThis PR turns the Tests
Final VerdictNeeds changes before merge: the logic looks reasonable, but the required Bugfix Validation evidence is still missing. LLVM Coverage Report
Changed lines: Changed C/C++ lines covered: 7/7 (100.00%) · Uncovered code |
clickhouse-gh
Bot
added
the
pr-bugfix
groeneai
added
can be tested
![clickhouse-gh[bot]](https://avatars.githubusercontent.com/in/928874?s=60&v=4){kind=small}
| CREATE MATERIALIZED VIEW v_view_target TO v_table_target AS SELECT a FROM src; | ||
|
|
||
| -- Inserting into a view whose target is another view is refused on both insert routes. | ||
| INSERT INTO v_view_target VALUES (1); -- { serverError NOT_IMPLEMENTED } |
There was a problem hiding this comment.
CI's merge-blocking new_tests_check.py hook still says none of the per-arch Bugfix Validation jobs validated this reproducer: the current Finish Workflow report shows all four per-arch jobs as DROPPED. For a Bug Fix PR that means we still do not have automated proof that master reproduces the failure and this patch fixes it, so this needs to be rewritten into a form Bugfix Validation can actually observe on the master debug/ASan builds, or the PR needs an explanation for why that evidence gate cannot cover this case.
Build profile diff (arm_release)Comparing ✅ No significant changes. Binary sizes
Only the stripped binary is compared: the official master build keeps debug symbols while PR builds strip them, so the other binaries differ by construction. Compile time of recompiled translation units7 translation units recompiled, 17 s compile time in total, 7 of them have a recent master baseline. |
There was a problem hiding this comment.
@groeneai can you check if the same error happens with dictionaries and refreshable materialized views? What about DELETE, UPDATE, and TRUNCATE statements? ALTERs seem safe, and I can't think of others.
There was a problem hiding this comment.
Checked all five on a debug build, both against pristine master and against this PR. One of them is a real carrier and is now covered; the rest are already refused elsewhere or do not reach the code at all.
Refreshable materialized views: yes, and it was uncovered. A refreshable view is a StorageMaterializedView, so it reaches the same target edge. Master aborts, this PR refuses:
CREATE MATERIALIZED VIEW v_refreshable REFRESH EVERY 1 YEAR APPEND TO tgt AS SELECT a FROM src;
CREATE MATERIALIZED VIEW v_refreshable_target TO v_refreshable AS SELECT a FROM src;
INSERT INTO v_refreshable_target VALUES (5);master exit 134, this PR Code: 48 NOT_IMPLEMENTED. There is also a fourth route into the hop: the refresh insert of a refreshable view whose own target forwards into a view. Master aborts there too; here the refusal lands in system.view_refreshes.exception and the server stays up. Added the first as a test cell (55f2479, APPEND so it needs no Atomic database, with a target table of its own so its initial refresh does not disturb the later counts). Confirmed live: the cell alone aborts master.
Dictionaries: no. A dictionary target is refused before the header is ever needed, identically on master and here, on both the direct and the dependent route: Code: 48, Method write is not supported by storage Dictionary. INSERT INTO dict directly gives the same. A dictionary also cannot be the addressed side of the edge, since the guard needs the insert to address a materialized view.
DELETE: no. Code: 36, DELETE query is not supported for table ..., both arms.
UPDATE: no. Code: 48, Table with engine MaterializedView doesn't support lightweight updates, both arms. ALTER TABLE ... DELETE is rejected earlier still: Code: 80, MATERIALIZED VIEW targets existing table ..., execute the statement directly on it.
TRUNCATE: no, and it never reaches an insert. StorageMaterializedView::truncate is gated on has_inner_table, so on any TO <target> view it is a no-op regardless of what the target is. Measured identical on both arms, and I checked it against a view whose target is an ordinary table before claiming it: that is also a no-op, while TRUNCATE on a plain table empties it. Pre-existing behaviour, unrelated to this PR.
ALTERs: agreed. ADD COLUMN is Code: 48 on a materialized view; MODIFY QUERY succeeds and does not reach the edge, including when used to retarget.
One thing I did not add. The refusal message names both views, but the test asserts only the error code. The only way to pin the text from a .sql test is system.errors.last_error_message, which is server-global: it reads correctly in isolation and returns the wrong answer as soon as any other NOT_IMPLEMENTED is raised on the same server, which a concurrent copy of this parallel-safe test does. I left the property unpinned rather than add an assertion I had measured to be flaky. Happy to convert the file to .sh and grep the diagnostic if you would rather have it checked.
A refreshable materialized view reaches the same target edge, so master aborts on an insert into a view that forwards into one. Measured: rc 134 on master, NOT_IMPLEMENTED here. The refresh insert of a refreshable view is a fourth route into the same hop and is refused the same way. APPEND mode is used so the cell needs no Atomic database, and the refreshable view gets a target table of its own so its initial refresh does not disturb the row counts the later assertions read.
The population insert throws after the view is created on the database engines that populate non-atomically, so `v_populate` survives an `Ordinary` database and a replicated DDL entry while it does not survive the default one. Measured one leaked row under `Ordinary`; every later assertion in the test held anyway, so this is hygiene rather than a failure. The comment called a skipped dependent-view target edge a stale dependency. The test creates that topology directly, so it is live from birth; describe the skip instead of its cause.
CI finish ledger - 55f2479CI is fully finished at this commit and no check failed, so no ledger row is owed. Evidence: 172 unique check runs, none queued or in progress; Config Workflow, Style check and Session id: cron:our-pr-ci-monitor:20260823-163000 |
robot-ch-test-poll
added
the
pr-synced-to-cloud
ReproducerCREATE TABLE src (a UInt16) ENGINE = MergeTree ORDER BY tuple();
CREATE TABLE tgt (a UInt16) ENGINE = MergeTree ORDER BY tuple();
CREATE MATERIALIZED VIEW v_table_target TO tgt AS SELECT a FROM src;
CREATE MATERIALIZED VIEW v_view_target TO v_table_target AS SELECT a FROM src;
INSERT INTO v_view_target VALUES (1);
-- 25.6 .. 26.7: Code: 1001. std::out_of_range: unordered_map::at: key not found. (STD_EXCEPTION)
-- SIGABRT (exit 134) on debug/sanitizer builds
-- 25.5 and older: insert succeeds, row forwarded to tgt
-- master (after #115985): Code: 48 NOT_IMPLEMENTED naming the offending viewResults:
Backport the fix to CC component owner: @CheSema @al13n321 Analysis metadata
Introducing change: not identified
|
|
@PedroTadim could you apply Affectedness, with the official per-branch binaries and the reproducer above ( 26.8 needs nothing: the merge commit Two things the generated PRs will need, from replaying the pick with the base
|
robot-ch-test-poll2
added
the
pr-must-backport-synced
…w whose target is a view The incoming hunk bundles the fix with master-only changes 26.3 does not have: the `materialized_view` block was hoisted above the map assignments there (26.3 still has the `select_table_id` check below them, so taking it would duplicate it), `getInMemoryMetadataPtr` takes `(context, bool)` on master against `(bool)` here, and the external dynamic metadata refresh is unrelated to this fix. Kept this branch's `getInMemoryMetadataPtr()` and placed the `NOT_IMPLEMENTED` throw in its own materialized-view block, in master's relative order. The throw lands after the map assignments here, which is equivalent: it aborts the insert and the builder is discarded. `NOT_IMPLEMENTED` is declared too, as master's file already had it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…hose target is a view
robot-clickhouse-ci-1
added
the
pr-backports-created
…hose target is a view
Backport #115985 to 26.6: Refuse an insert into a materialized view whose target is a view
…hose target is a view
Backport #115985 to 26.7: Refuse an insert into a materialized view whose target is a view
Backport #115985 to 26.3: Refuse an insert into a materialized view whose target is a view
6 participants
Closes: #114494
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Fixes a server abort in debug and sanitizer builds when inserting into a materialized view whose
TOtarget is another materialized view. Such an insert already failed on released versions, with an uninformativeCode: 1001 STD_EXCEPTION; it now reportsNOT_IMPLEMENTEDnaming the offending view.Description
Found by the AST fuzzer.
INSERTinto a materialized view declaredTO <another materialized view>aborts:observePathrejects a hop whose view does not select from its parent. That check is for a stale dependency left behind by anALTER, and before the pushing-to-views rework it only ran while iterating dependent views. It is now also reached from the target hop, where a view never selects from the view forwarding into it, so it always rejects: the path is abandoned, no output header is recorded, andcreatePreSinkcallsoutput_headers.at(view_id)on a missing key. Guarding that.at()alone is not enough, because the hop has no sink either (createSinkImplreachesUNREACHABLE()for a materialized view inner storage).This refuses the insert with
NOT_IMPLEMENTED, in the shapecreateRetryalready uses for a forwarding path it cannot rebuild.The refusal is limited to the view the insert addresses, on all three routes that reach the abort:
INSERT ... VALUES,INSERT ... SELECT(viaaddInsertToSelectPipeline) andCREATE ... POPULATE(via the population insert). A view reached as a dependent still gets the existing stale-dependency skip, which is what04216_materialized_view_reused_target_name_dependencyasserts; on that route the rows are still dropped silently, so the data-loss half of #114494 is not addressed here.Validated against a pristine master build: all three routes abort there and return
NOT_IMPLEMENTEDhere, an ordinary-table target is byte-identical on both, and the new test fails on master (the server exits 134 on the first insert).04652_nested_merge_prewhere_type_mismatch, the fuzzer's own carrier, creates this shape and still passes.Workflow [PR]
Sync PR [sync-upstream/pr/115985]
Version info
26.7.7.95,26.6.6.4,26.3.33.72