Repository navigation
ore,mzd,slt: improve ergonomics of server shutdown - #953
Merged
Merged
Conversation
Shutting down a materialized server must be done carefully. First the command queue transmitter (cmdq_tx) must be shutdown, to send the shutdown signal. Then we have to wait for the dataflow workers to shutdown, in order to make sure messages are not still flowing between the workers and the coordinator. Then we have to wait for the coordinator to shut down, to make sure nothing is still using the tokio runtime. And lastly, the tokio runtime can be shutdown. This was previously handled with a custom drop implementation for materialized::Server, and a drop-like shutdown function for sqllogictest::runner::State. These approaches were both flawed; materialized::Server was forced to use a bunch of unnecessary options so that it could move out of some fields in its drop implementation, while sqllogictest was forgetting to call the shutdown method in some modes. Solve both issues by providing a JoinOnDropHandle that waits for a thread to shutdown when the handle is dropped. (std::thread::JoinHandle *detaches* from the thread when dropped.) Then we don't need to provide a custom drop implementation, but can rely on the field drop order to handle everything for us. This has the nice side effect of solving the hang when passing --rewrite-results to SLT.
mgree
added a commit
that referenced
this pull request
Oct 28, 2026
### Motivation Closes https://linear.app/materializeinc/issue/SQL-715/drop-index-with-dependents-should-not-be-allowed. This corrects a longstanding mis-feature that will become more dire once we have pinned LIR. ### Description Orphaned dataflows are a confusing mis-feature: since indexes aren't first-class SQL objects, they could be freely dropped. That's all well and good for an OLTP database, but not so an operational one. This PR forbids orphaned dataflows. `DROP INDEX` now fails with `DEPENDENT_OBJECTS_STILL_EXIST` (`2BP01`) when a materialized view, index, or metric sink reads from the index, and the error names the readers. When one statement drops several indexes, one error names every blocked index. `DROP INDEX ... CASCADE` keeps its existing meaning, the same as `RESTRICT`: it fails the same way, and there is no way to drop all dependents of an index in one statement. Users drop the readers themselves, or replace them with a blue/green deployment. A newer index on a relation can read from an older one on the same relation, so indexes on one relation now have to be dropped newest first. An `enable_drop_in_use_index` flag defaults to off, but allows the old behavior, with a `WARNING` notice, an error-level log event, and a metric increment. The Terraform provider up to v0.11.10 replaces an index by running `DROP INDEX ... RESTRICT` first, so replacing a `materialize_index` that a materialized view reads fails with the in-use error on every apply, and `terraform destroy` fails intermittently because the provider drops the view and the index in parallel. [terraform-provider-materialize#952](MaterializeInc/terraform-provider-materialize#952) and [#953](MaterializeInc/terraform-provider-materialize#953) fix the provider. Until users upgrade, adding `lifecycle { replace_triggered_by = [materialize_index.<name>.id] }` to the materialized view is the workaround. In-flight transient dataflows (`SUBSCRIBE`, `COPY TO`, and `SELECT`) never block a drop and are reported in a notice instead. ### Verification New tests - test/sqllogictest/drop_index_dependents.slt covers: - A plain DROP INDEX or DROP INDEX ... RESTRICT on an index a materialized view reads from fails. The test checks the full error, DETAIL and HINT, and that nothing was dropped. - DROP INDEX ... CASCADE fails the same way while readers exist, and succeeds once they are gone. - An index whose dataflow reads from another index also counts as a dependent (sibling indexes, and v_idx on t_idx). - A dependent that the same statement also drops doesn't block it (DROP INDEX a, b and DROP SCHEMA s CASCADE). - Dropping several blocked indexes at once reports all of them in one error. - With enable_drop_in_use_index on, a plain drop or a CASCADE drop removes only the index and leaves the view running and queryable. - test/testdrive/drop-index-dependents.td checks that no drop leaves an orphaned dataflow in mz_compute_dependencies, including CASCADE drops of the relation, schema, and cluster. With the flag on, it confirms the check detects the orphan. - src/environmentd/tests/sql.rs adds three Rust integration tests: - test_drop_in_use_index_is_refused: checks the SQLSTATE, message and hint, and that a CASCADE drop fails with the same SQLSTATE. - test_drop_in_use_index_flag_warns: turns the flag on at startup, checks the WARNING notice's message, detail and hint, and that the view survives. - test_drop_index_read_by_in_flight_subscribe_notice: an open SUBSCRIBE cursor doesn't block the drop and produces the expected NOTICE. Existing tests changed to match the new behavior. Tests drop the readers explicitly instead of orphaning a dataflow, unless they test orphans themselves. - Drop order swapped so readers go first: cluster.slt, privilege_checks.slt, object_ownership.slt, indexes.td, divergent-dataflow-cancellation.td, metric-sink.td, and the restart (arrangement-sizes-stale-snapshot-after-restart) and cluster (test-replica-metrics, test-compute-controller-metrics) workflows. - Readers dropped and recreated around the drop: hydration-status.td, materializations.td, rename.td, join_index.slt, and the explain slts (default, optimized_plan_as_text, optimized_plan_as_text_redacted). - 0dt basic and ddl drop a dedicated index that nothing reads, so the view the test follows across the handoff keeps reading its original index. - pgtest-mz/notice.pt now checks the refusals and their messages instead of the old NOTICE. - The flag is turned on only in tests that deliberately orphan a dataflow, with a comment saying so: explain/materialized_view.slt, pushdown.slt, index_already_exists.slt, introspection-sources.td, and cluster test-compute-reconciliation-replace. Other test frameworks - Platform check DropIndex drops the view before the indexes, newest first, and recreates the view. - Parallel workload DropIndexAction: ignores the new "still depended upon by" error. - mzcompose: enable_drop_in_use_index is listed as an uninteresting parameter, so CI runs with it off. Gaps - No test covers the flag's error-level log event or its metric increment. - No test covers the NOTICE for in-flight COPY TO or SELECT. Only SUBSCRIBE is tested. - No restart or upgrade test checks that pinned plans survive once orphaning is blocked. That is the problem the change fixes. --------- Co-authored-by: Claude <[email protected]> Co-authored-by: Jon Currey <[email protected]>
ggevay
added a commit
that referenced
this pull request
Nov 4, 2026
…doc (#40475) Follow-up to #39098: the Terraform paragraph it added to the design doc has two errors. I've corrected the same paragraph in #39098's description, since the release notes are generated from it. - **Affected versions:** it said "up to v0.11.10". Every released provider version fails the replacement: v0.11.11 and v0.11.12 fail it the same way, and v0.11.13 has no index changes. - **What fixes it:** it said that [terraform-provider-materialize#952](MaterializeInc/terraform-provider-materialize#952) and [#953](MaterializeInc/terraform-provider-materialize#953) fix the provider, and that `replace_triggered_by` is the workaround until users upgrade. With both, replacing an index that a materialized view reads still fails, after a minute of retries. [#952](MaterializeInc/terraform-provider-materialize#952) only helps when the readers are dropped in the same run, such as in `terraform destroy`. `replace_triggered_by` is the way to replace such an index with any provider version, and [#953](MaterializeInc/terraform-provider-materialize#953) documents it. Verified against #39098's CI image with provider v0.11.11, v0.11.12 and a build of [terraform-provider-materialize#953](MaterializeInc/terraform-provider-materialize#953), which includes [#952](MaterializeInc/terraform-provider-materialize#952). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5.5 <[email protected]>
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
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.
Shutting down a materialized server must be done carefully. First the
command queue transmitter (cmdq_tx) must be shutdown, to send the
shutdown signal. Then we have to wait for the dataflow workers to
shutdown, in order to make sure messages are not still flowing between
the workers and the coordinator. Then we have to wait for the
coordinator to shut down, to make sure nothing is still using the tokio
runtime. And lastly, the tokio runtime can be shutdown.
This was previously handled with a custom drop implementation for
materialized::Server, and a drop-like shutdown function for
sqllogictest::runner::State. These approaches were both flawed;
materialized::Server was forced to use a bunch of unnecessary options so
that it could move out of some fields in its drop implementation, while
sqllogictest was forgetting to call the shutdown method in some modes.
Solve both issues by providing a JoinOnDropHandle that waits for a
thread to shutdown when the handle is dropped. (std::thread::JoinHandle
detaches from the thread when dropped.) Then we don't need to provide
a custom drop implementation, but can rely on the field drop order to
handle everything for us.
This has the nice side effect of solving the hang when passing
--rewrite-results to SLT.