Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 20, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 57 additions & 3 deletions src/adapter/src/coord/sequencer/inner.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,12 @@ use mz_expr::{
RowSetFinishing,
};
use mz_ore::collections::CollectionExt;
use mz_ore::task;
use mz_ore::tracing::OpenTelemetryContext;
use mz_ore::vec::VecExt;
use mz_ore::{soft_assert, task};
use mz_repr::adt::jsonb::Jsonb;
use mz_repr::adt::mz_acl_item::{MzAclItem, PrivilegeMap};
use mz_repr::explain::{ExplainFormat, UsedIndexes};
use mz_repr::explain::{ExplainFormat, ExprHumanizer, UsedIndexes};
use mz_repr::role_id::RoleId;
use mz_repr::{Datum, Diff, GlobalId, RelationDesc, RelationType, Row, RowArena, Timestamp};
use mz_sql::ast::{ExplainStage, IndexOptionName};
Expand Down Expand Up @@ -105,7 +105,7 @@ use crate::coord::{
use crate::error::AdapterError;
use crate::explain::explain_dataflow;
use crate::explain::optimizer_trace::OptimizerTrace;
use crate::notice::AdapterNotice;
use crate::notice::{AdapterNotice, DroppedInUseIndex};
use crate::session::{EndTransactionAction, Session, TransactionOps, TransactionStatus, WriteOp};
use crate::subscribe::ActiveSubscribe;
use crate::util::{viewable_variables, ClientTransmitter, ComputeSinkId, ResultExt};
Expand All @@ -128,6 +128,7 @@ struct DropOps {
ops: Vec<catalog::Op>,
dropped_active_db: bool,
dropped_active_cluster: bool,
dropped_in_use_indexes: Vec<DroppedInUseIndex>,
}

// A bundle of values returned from create_source_inner
Expand Down Expand Up @@ -1232,6 +1233,7 @@ impl Coordinator {
ops,
dropped_active_db,
dropped_active_cluster,
dropped_in_use_indexes,
} = self.sequence_drop_common(session, drop_ids)?;

self.catalog_transact(Some(session), ops).await?;
Expand All @@ -1248,6 +1250,9 @@ impl Coordinator {
name: session.vars().cluster().to_string(),
});
}
for dropped_in_use_index in dropped_in_use_indexes {
session.add_notice(AdapterNotice::DroppedInUseIndex(dropped_in_use_index));
}
Ok(ExecuteResponse::DroppedObject(object_type))
}

Expand Down Expand Up @@ -1465,6 +1470,7 @@ impl Coordinator {
ops: drop_ops,
dropped_active_db,
dropped_active_cluster,
dropped_in_use_indexes,
} = self.sequence_drop_common(session, plan.drop_ids)?;

let ops = privilege_revoke_ops
Expand All @@ -1484,6 +1490,9 @@ impl Coordinator {
name: session.vars().cluster().to_string(),
});
}
for dropped_in_use_index in dropped_in_use_indexes {
session.add_notice(AdapterNotice::DroppedInUseIndex(dropped_in_use_index));
}
Ok(ExecuteResponse::DroppedOwned)
}

Expand All @@ -1494,6 +1503,7 @@ impl Coordinator {
) -> Result<DropOps, AdapterError> {
let mut dropped_active_db = false;
let mut dropped_active_cluster = false;
let mut dropped_in_use_indexes = Vec::new();
let mut dropped_roles = BTreeMap::new();
let mut dropped_databases = BTreeSet::new();
let mut dropped_schemas = BTreeSet::new();
Expand All @@ -1505,6 +1515,7 @@ impl Coordinator {
// database or schema.
let mut default_privilege_revokes = BTreeSet::new();

let ids_set = ids.iter().collect::<BTreeSet<_>>();
for id in &ids {
match id {
ObjectId::Database(id) => {
Expand Down Expand Up @@ -1540,6 +1551,42 @@ impl Coordinator {
role_revokes.insert((*group_id, *id, *grantor_id));
}
}
ObjectId::Item(id) => {
if let Some(index) = self.catalog().get_entry(id).index() {
let humanizer = self.catalog().for_session(session);
let dependants = self
.controller
.compute
.collection_reverse_dependencies(index.cluster_id, *id)
.ok()
.into_iter()
.flatten()
.filter(|dependant_id| {
// Transient Ids belong to Peeks. We are not interested for now in
// peeks depending on a dropped index.
// TODO: show a different notice in this case. Something like
// "There is an in-progress ad hoc SELECT that uses the dropped
// index. The resources used by the index will be freed when all
// such SELECTs complete."
!matches!(dependant_id, GlobalId::Transient(..)) &&
// If the dependent object is also being dropped, then there is no
// problem, so we don't want a notice.
!ids_set.contains(&ObjectId::Item(**dependant_id))
})
.map(|dependant_id| {
humanizer
.humanize_id(*dependant_id)
.unwrap_or(id.to_string())
})
.collect_vec();
if !dependants.is_empty() {
dropped_in_use_indexes.push(DroppedInUseIndex {
index_name: humanizer.humanize_id(*id).unwrap_or(id.to_string()),
dependant_objects: dependants,
});
}
}
}
_ => {}
}
}
Expand Down Expand Up @@ -1602,6 +1649,7 @@ impl Coordinator {
ops,
dropped_active_db,
dropped_active_cluster,
dropped_in_use_indexes,
})
}

Expand Down Expand Up @@ -4206,13 +4254,19 @@ impl Coordinator {
mut ops,
dropped_active_db,
dropped_active_cluster,
dropped_in_use_indexes,
} = self.sequence_drop_common(session, drops)?;

assert!(
!dropped_active_db && !dropped_active_cluster,
"dropping subsources does not drop DBs or clusters"
);

soft_assert!(
dropped_in_use_indexes.is_empty(),
"Dropping subsources might drop indexes, but then all objects dependent on the index should also be dropped."
);

// Redefine source.
ops.push(catalog::Op::UpdateItem {
id,
Expand Down
17 changes: 16 additions & 1 deletion src/adapter/src/notice.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use std::fmt;
use chrono::{DateTime, Utc};
use mz_controller::clusters::ClusterStatus;
use mz_orchestrator::{NotReadyReason, ServiceStatus};
use mz_ore::str::StrExt;
use mz_ore::str::{separated, StrExt};
use mz_repr::adt::mz_acl_item::AclMode;
use mz_repr::strconv;
use mz_sql::ast::NoticeSeverity;
Expand Down Expand Up @@ -117,6 +117,7 @@ pub enum AdapterNotice {
WebhookSourceCreated {
url: url::Url,
},
DroppedInUseIndex(DroppedInUseIndex),
}

impl AdapterNotice {
Expand Down Expand Up @@ -154,6 +155,7 @@ impl AdapterNotice {
.into(),
),
AdapterNotice::OptimizerNotice { notice: _, hint } => Some(hint.clone()),
AdapterNotice::DroppedInUseIndex(..) => Some("To free up the resources used by the index, recreate all the above-mentioned objects.".into()),
_ => None
}
}
Expand Down Expand Up @@ -196,6 +198,7 @@ impl AdapterNotice {
},
AdapterNotice::UnknownSessionDatabase(_) => SqlState::SUCCESSFUL_COMPLETION,
AdapterNotice::OptimizerNotice { .. } => SqlState::SUCCESSFUL_COMPLETION,
AdapterNotice::DroppedInUseIndex { .. } => SqlState::WARNING,
AdapterNotice::WebhookSourceCreated { .. } => SqlState::WARNING,
}
}
Expand Down Expand Up @@ -342,10 +345,22 @@ impl fmt::Display for AdapterNotice {
AdapterNotice::WebhookSourceCreated { url } => {
write!(f, "URL to POST data is '{url}'")
}
AdapterNotice::DroppedInUseIndex(DroppedInUseIndex {
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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@ggevay ggevay Sep 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@aalexandrov aalexandrov Sep 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@aalexandrov aalexandrov Sep 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest

until they are dropped, altered or Materialize is restarted

in order to also cover #20725.

Copy link
Contributor Author

@ggevay ggevay Sep 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!"

}
}
}
}

#[derive(Clone, Debug)]
pub struct DroppedInUseIndex {
pub index_name: String,
pub dependant_objects: Vec<String>,
}

impl From<PlanNotice> for AdapterNotice {
fn from(notice: PlanNotice) -> AdapterNotice {
AdapterNotice::PlanNotice(notice)
Expand Down
1 change: 1 addition & 0 deletions src/adapter/src/severity.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,7 @@ impl Severity {
AdapterNotice::UnknownSessionDatabase(_) => Severity::Notice,
AdapterNotice::OptimizerNotice { .. } => Severity::Notice,
AdapterNotice::WebhookSourceCreated { .. } => Severity::Notice,
AdapterNotice::DroppedInUseIndex { .. } => Severity::Notice,
}
}
}
Expand Down
11 changes: 11 additions & 0 deletions src/compute-client/src/controller.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,17 @@ impl<T> ComputeController<T> {
storage,
}
}

/// List compute collections that depend on the given collection.
pub fn collection_reverse_dependencies(
&self,
instance_id: ComputeInstanceId,
id: GlobalId,
) -> Result<impl Iterator<Item = &GlobalId>, InstanceMissing> {
Ok(self
.instance(instance_id)?
.collection_reverse_dependencies(id))
}
}

impl<T> ComputeController<T>
Expand Down
11 changes: 11 additions & 0 deletions src/compute-client/src/controller/instance.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,17 @@ impl<T> Instance<T> {
};
self.ready_responses.push_back(resp);
}

/// List compute collections that depend on the given collection.
pub fn collection_reverse_dependencies(&self, id: GlobalId) -> impl Iterator<Item = &GlobalId> {
self.collections_iter().filter_map(move |(id2, state)| {
if state.compute_dependencies.contains(&id) {
Some(id2)
} else {
None
}
})
}
}

impl<T> Instance<T>
Expand Down
85 changes: 85 additions & 0 deletions test/pgtest-mz/notice.pt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,90 @@
# Test various NOTICE expectations.

send
Query {"query": "create table t(a int, b int)"}
Query {"query": "create index t_idx_a on t(a)"}
Query {"query": "create materialized view mv1 as select * from t where b=7"}
Query {"query": "create view v1 as select * from t union select * from t"}
Query {"query": "create index v1_idx_a on v1(a)"}
Query {"query": "drop index v1_idx_a"}
Query {"query": "drop index t_idx_a"}
Query {"query": "drop materialized view mv1"}
Query {"query": "create index v1_idx_a on v1(a)"}
Query {"query": "create materialized view mv2 as select * from t where a=5"}
Query {"query": "drop index v1_idx_a"}
Query {"query": "drop materialized view mv2"}
Query {"query": "create index t_idx_a on t(a)"}
Query {"query": "create materialized view mv2 as select * from t where a=5"}
Query {"query": "drop index t_idx_a"}
Query {"query": "create index t_idx_a on t(a)"}
Query {"query": "create index v1_idx_a on v1(a)"}
Query {"query": "drop index t_idx_a"}
Query {"query": "drop index v1_idx_a"}
----

until
ReadyForQuery
ReadyForQuery
ReadyForQuery
ReadyForQuery
ReadyForQuery
ReadyForQuery
ReadyForQuery
ReadyForQuery
ReadyForQuery
ReadyForQuery
ReadyForQuery
ReadyForQuery
ReadyForQuery
ReadyForQuery
ReadyForQuery
ReadyForQuery
ReadyForQuery
ReadyForQuery
ReadyForQuery
----
CommandComplete {"tag":"CREATE TABLE"}
ReadyForQuery {"status":"I"}
CommandComplete {"tag":"CREATE INDEX"}
ReadyForQuery {"status":"I"}
CommandComplete {"tag":"CREATE MATERIALIZED VIEW"}
ReadyForQuery {"status":"I"}
CommandComplete {"tag":"CREATE VIEW"}
ReadyForQuery {"status":"I"}
CommandComplete {"tag":"CREATE INDEX"}
ReadyForQuery {"status":"I"}
CommandComplete {"tag":"DROP INDEX"}
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.mv1. The index will be dropped from the catalog, but it will continue to be maintained and take up resources!"}]}
CommandComplete {"tag":"DROP INDEX"}
ReadyForQuery {"status":"I"}
CommandComplete {"tag":"DROP MATERIALIZED VIEW"}
ReadyForQuery {"status":"I"}
CommandComplete {"tag":"CREATE INDEX"}
ReadyForQuery {"status":"I"}
CommandComplete {"tag":"CREATE MATERIALIZED VIEW"}
ReadyForQuery {"status":"I"}
CommandComplete {"tag":"DROP INDEX"}
ReadyForQuery {"status":"I"}
CommandComplete {"tag":"DROP MATERIALIZED VIEW"}
ReadyForQuery {"status":"I"}
CommandComplete {"tag":"CREATE INDEX"}
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!"}]}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

CommandComplete {"tag":"DROP INDEX"}
ReadyForQuery {"status":"I"}
CommandComplete {"tag":"CREATE INDEX"}
ReadyForQuery {"status":"I"}
CommandComplete {"tag":"CREATE INDEX"}
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.v1_idx_a. The index will be dropped from the catalog, but it will continue to be maintained and take up resources!"}]}
CommandComplete {"tag":"DROP INDEX"}
ReadyForQuery {"status":"I"}
CommandComplete {"tag":"DROP INDEX"}
ReadyForQuery {"status":"I"}

# Test setting session variables to nonexistant values
# Scenarios tested: nonexistant values, exstant values, capitalized variables
# double quotes, single quotes, default values
Expand Down