Give a notice when dropping an in-use index #21706

ggevay · 2023-09-11T23:43:54Z

This is the first step of addressing https://github.com/MaterializeInc/database-issues/issues/6111: We give a notice when the user drops an index that is in use.

I'm planning to open a follow-up PR that will add some documentation about this situation, and will link to that in the notice message.

Note that the computation of reverse_compute_dependencies is not very efficient, because it goes through all collections. But we decided with Jan that this is ok, and it's not worth it to make it more complicated for efficiency, because this should still take less than 1 ms, and dropping an index is not performance critical.

Motivation

This PR adds a known-desirable feature: First step of https://github.com/MaterializeInc/database-issues/issues/6111

Tips for reviewer

Checklist

This PR has adequate test coverage / QA involvement has been duly considered.
- Nightly: https://buildkite.com/materialize/nightlies/builds/4102
This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
If this PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way), then it is tagged with a T-proto label.
If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
This PR includes the following user-facing behavior changes:

umanwizard

Thanks!

umanwizard · 2023-09-19T15:29:31Z

src/adapter/src/notice.rs

+                index_name,
+                dependant_objects,
+            }) => {
+                write!(f, "The dropped index {index_name} is being used by the following objects: {}. The index will be dropped from the catalog, but it will continue to be maintained and take up resources!", separated(", ", dependant_objects))


Should we also say something about the fact that the index will cease to exist on next restart?

The hint() for this notice variant mentions this.

We discussed this and this seems the fact that we re-plan during the maintenance window is incidental, so I'm not sure if we should mention this here.

The hint says
"To free up the resources used by the index, recreate all the above-mentioned objects."

We were thinking with Alex to not emphasize the maintenance window restart too much, because it's just an incidental thing, e.g., it might not occur if there is no release on a certain week. Also, we don't want users waiting for the maintenance window with the replanning, because then if the index disappearing creates a performance problem then we'd get confused whether it was the version upgrade or just an index being dropped.

But I'm planning to open a follow-up PR with adding docs for these notices, and there I will probably mention this anyway.

I agree about not mentioning the maintenance window here. It would be weird to have the database code make assumptions about how we operate Materialize.

We could mention Materialize restarts in a generic way (which is what Brennan suggested I think) but then there is nothing actionable the user can do with that information, so I don't see how it would be useful.

I think it's important for the user to be aware that at some point in the future, outside of their control, the objects using this index will become less efficient as they will no longer have access to the index. We don't need to specifically mention maintenance windows, I guess.

Well, I can imagine this happening, unfortunately.

This seems like an anti-pattern. Until we implement plan pinning as a feature that is enabled by default for every installed materialized view and index dataflow, users should not rely on their plans being stable across restarts, especially if they change the environment in the meantime.

I agree that users should not be relying on this, but unfortunately I think some will rely on it unless we explicitly teach them not to.

Ok, so what should be the wording? This is the current wording:

"The dropped index {index_name} is being used by the following objects: {}. The index will be dropped from the catalog, but it will continue to be maintained and take up resources!"

we could instead have

"The dropped index {index_name} is being used by the following objects (until they are dropped or Materialize is restarted): {}. The index is now dropped from the catalog, but it will continue to be maintained and take up resources!"

What do you all think?

I suggest

until they are dropped, altered or Materialize is restarted

in order to also cover #20725.

I've opened a PR with the following:

"The dropped index {index_name} is being used by the following objects: {}. The index is now dropped from the catalog, but it will continue to be maintained and take up resources until all dependent objects are dropped, altered, or Materialize is restarted!"

aalexandrov

Some minor nits, otherwise looks good.

aalexandrov · 2023-09-19T16:00:48Z

test/pgtest-mz/notice.pt

+ReadyForQuery {"status":"I"}
+CommandComplete {"tag":"CREATE MATERIALIZED VIEW"}
+ReadyForQuery {"status":"I"}
+NoticeResponse {"fields":[{"typ":"S","value":"NOTICE"},{"typ":"C","value":"01000"},{"typ":"M","value":"The dropped index materialize.public.t_idx_a is being used by the following objects: materialize.public.mv2. The index will be dropped from the catalog, but it will continue to be maintained and take up resources!"}]}


I'm not sure what the difference in the setup between the two notices is. Can we have one which tests the "index being used in the dataflow maintaining another index" path?

src/compute-client/src/controller/instance.rs

src/adapter/src/coord/sequencer/inner.rs

aalexandrov · 2023-09-19T16:36:49Z

src/adapter/src/notice.rs

+                index_name,
+                dependant_objects,
+            }) => {
+                write!(f, "The dropped index {index_name} is being used by the following objects: {}. The index will be dropped from the catalog, but it will continue to be maintained and take up resources!", separated(", ", dependant_objects))


The hint() for this notice variant mentions this.

We discussed this and this seems the fact that we re-plan during the maintenance window is incidental, so I'm not sure if we should mention this here.

teskje

LGTM, just a couple nits!

src/adapter/src/coord/sequencer/inner.rs

teskje · 2023-09-20T11:55:01Z

src/adapter/src/notice.rs

+                index_name,
+                dependant_objects,
+            }) => {
+                write!(f, "The dropped index {index_name} is being used by the following objects: {}. The index will be dropped from the catalog, but it will continue to be maintained and take up resources!", separated(", ", dependant_objects))


I agree about not mentioning the maintenance window here. It would be weird to have the database code make assumptions about how we operate Materialize.

We could mention Materialize restarts in a generic way (which is what Brennan suggested I think) but then there is nothing actionable the user can do with that information, so I don't see how it would be useful.

src/compute-client/src/controller.rs

src/compute-client/src/controller/instance.rs

ggevay force-pushed the drop-used-index-notice branch from 13c7ffc to 915fe60 Compare September 11, 2023 23:48

ggevay force-pushed the drop-used-index-notice branch 2 times, most recently from cd8f6e1 to b257446 Compare September 19, 2023 11:38

ggevay added A-optimization Area: query optimization and transformation A-compute Area: compute labels Sep 19, 2023

ggevay marked this pull request as ready for review September 19, 2023 11:59

ggevay requested a review from a team September 19, 2023 11:59

ggevay requested a review from a team as a code owner September 19, 2023 11:59

ggevay requested a review from ParkMyCar September 19, 2023 11:59

umanwizard approved these changes Sep 19, 2023

View reviewed changes

aalexandrov approved these changes Sep 19, 2023

View reviewed changes

ggevay force-pushed the drop-used-index-notice branch from b257446 to 41d348e Compare September 20, 2023 11:46

teskje approved these changes Sep 20, 2023

View reviewed changes

ggevay force-pushed the drop-used-index-notice branch from 41d348e to 486d068 Compare September 20, 2023 12:36

Give a notice when dropping an in-use index

daca983

ggevay force-pushed the drop-used-index-notice branch from 486d068 to daca983 Compare September 20, 2023 12:40

ParkMyCar approved these changes Sep 20, 2023

View reviewed changes

ggevay merged commit f176f13 into MaterializeInc:main Sep 20, 2023

ggevay mentioned this pull request Sep 21, 2023

Add metrics for optimization notices #21868

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Give a notice when dropping an in-use index #21706

Give a notice when dropping an in-use index #21706

ggevay commented Sep 11, 2023 •

edited

Loading

umanwizard left a comment

umanwizard Sep 19, 2023

aalexandrov Sep 19, 2023

ggevay Sep 20, 2023 •

edited

Loading

teskje Sep 20, 2023

umanwizard Sep 20, 2023

aalexandrov Sep 20, 2023 •

edited

Loading

umanwizard Sep 20, 2023

ggevay Sep 20, 2023

aalexandrov Sep 20, 2023 •

edited

Loading

ggevay Sep 21, 2023 •

edited

Loading

aalexandrov left a comment •

edited

Loading

aalexandrov Sep 19, 2023

ggevay Sep 20, 2023

aalexandrov Sep 19, 2023

teskje left a comment

teskje Sep 20, 2023

Give a notice when dropping an in-use index #21706

Give a notice when dropping an in-use index #21706

Conversation

ggevay commented Sep 11, 2023 • edited Loading

Motivation

Tips for reviewer

Checklist

umanwizard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggevay Sep 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aalexandrov Sep 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aalexandrov Sep 20, 2023 • edited Loading

Choose a reason for hiding this comment

ggevay Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

aalexandrov left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teskje left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggevay commented Sep 11, 2023 •

edited

Loading

ggevay Sep 20, 2023 •

edited

Loading

aalexandrov Sep 20, 2023 •

edited

Loading

aalexandrov Sep 20, 2023 •

edited

Loading

ggevay Sep 21, 2023 •

edited

Loading

aalexandrov left a comment •

edited

Loading