doc/developer: add "Catalog tables for Optimizer Notices" design doc

[aalexandrov]

Design doc for the infrastructure requested in MaterializeInc/database-issues#6468.

Rendered version.

Motivation

• This PR adds a known-desirable feature.

Tips for reviewers

Still needs some sections to be filled in. I hope to move it out from draft mode later today.

• @ggnall / @sthm please make sure that the problem statement and the UX diagram are aligned with your vision / feature requests.
• @ggnall I'm also looking into guidance about the RBAC issues alternatives from the Open questions section.
• @ggevay as the primary reviewer of adapter: add catalog tables for optimizer notices #23360 the most relevant parts for you are the Catalog Schema and Schema Maintenance parts of the Solution Proposal.
• @ParkMyCar mainly I want to double-check for possible performance regressions in the way I intend to do mz_all_optimizer_notices maintenance.
• @aljoscha please review the Changes required for Platform V2 and let me know if this aligns with your vision for distributed cooordd.

Checklist

• This PR has adequate test coverage / QA involvement has been duly considered.
• 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:
  • There are no user-facing behavior changes.

[ggevay]

I wrote some comments. I'm still going through the design doc.

[ggevay]

On the other hand

[ggevay]

(For the corrective action of IndexTooWideForLiteralConstraints, we should mention that the user might want to drop the old index if it is not used by some other queries.)

[aalexandrov]

Are you with OK with just emitting a dedicated notice for indexes that are not used at all?

Then handling the IndexTooWideForLiteralConstraints and re-creating the objects might result in another notice IndexNotUsed which the user will have to ignore if they have created the index for serving purposes.

[ggevay]

This is tricky, because we'd like to keep notices quite high-signal, i.e., false positives should be rare. But IndexNotUsed would definitely show a false positive for each serving index, if we implement it by only looking at static objects. So, I'd say we should add this notice only when we'll also have the last usage time of each index implemented (https://github.com/MaterializeInc/materialize/issues/20877), because then we can take that also into account when deciding whether to show an IndexNotUsed.

[ggevay]

Are you with OK with just emitting a dedicated notice for indexes that are not used at all?

Ok! And then when we have IndexNotUsed later implemented in the above way, then these things will nicely fall into place.

[aalexandrov]

So, I'd say we should add this notice only when we'll also have the last usage time of each index implemented (https://github.com/MaterializeInc/materialize/issues/20877), because then we can take that also into account when deciding whether to show an IndexNotUsed.

The design also provisions for ignoring a notice in a way that will survive restarts. If this is the case the user can just manually ignore all false positive notices for IndexNotUsed.

[ggevay]

I've been thinking that maybe instead of "Optimizer notices" we should call these things "Optimization notices", because some of them are actually emitted not by the optimizer. For example, DroppedInUseIndex is created in sequence_drop_common.

[aalexandrov]

My impression was that the current iteration focuses precisely on the notices emitted by the optimizer as well as other notices that we can add in the future. For example, dropping an index that is still in use will create dangling import_ids in some of the plans of running indexes, so we can have other notices attached to the specific index or materialized view that depends on that index.

[aalexandrov]

I will rename this to "Optimization notices".

[ggevay]

We are recording action_type because at some point in the future we might want to add a button that simply executes it if it is sql_statements?

[aalexandrov]

Yes (this request came from @sthm). A button probably won't cut it until we have zero-downtime REPLAN which seems some way down the road. However, I guess even now it is better UX if the customer can copy-paste a valid SQL string the notice into their dbt project or whatever else they use to populate their catalog.

[ggevay]

I'm thinking about the exact connection between object_id and dependency_ids. For an example, let's consider the CommonArrangedInput notice (I just made up this name; it's https://github.com/MaterializeInc/materialize/issues/20802).

In this example, would dependency_ids list all those objects that arrange the same one input? But then if one of these objects is dropped, the notice might still be valid if originally there were more than two objects arranging the same input. It becomes invalid only if all but one of the ids in dependency_ids is dropped.

Another question for this example: what will be the object_id? Will we have one row for each object in dependency_ids?

[aalexandrov]

For an example, let's consider the CommonArrangedInput notice (I just made up this name; it's https://github.com/MaterializeInc/materialize/issues/20802).

Here is my idea:

• The notice is scoped under multiple catalog objects at once (so object_id is null).
• We have a background thread that periodically scans all catalog plans and extracts an index of such notices, one for each index.
• The notice is referring to objects that need to exist in order to make it valid. Rather it is suggesting an index that will be picked up by different dataflows, so we are collecting dependants and not dependencies.
• The dependants can be modeled in memory as a field CommonArrangedInput::dependant_ids: BTreeSet<GlobalId>. - When we "resolve" CommonArrangedInput into an OptimzierNotice we will render a message containing a list of fully-qualified names of these ids.
• When one of these dependant_ids entries is dropped we will remove it from the in-memory state of associated CommonArrangedInput, emit a retraction for the old mz_all_optimzier_notices for these notices, and an insertion with the new state.
• When the dependant_ids becomes a singleton or with only one entry we will emit a retraction for the old notice and remove it from the in-memory state.

[aalexandrov]

Maybe another way to model this is to have dependant_ids (as a separate relation or a list) instead of object_id. This will contain one entry for notices scoped to a single element and multiple entries for things like CommonArrangedInput.

[ggevay]

Hmm, ok, so this will be used by Rust code in a custom way for each notice when computing table updates, and not by some SQL in a view that just simply hides the row when any (or all) of the things in dependency_ids is gone, right?

[ggevay]

Maybe another way to model this is to have dependant_ids (as a separate relation or a list) instead of object_id. This will contain one entry for notices scoped to a single element and multiple entries for things like CommonArrangedInput.

Sounds good.

[aalexandrov]

The Eager cleanup of notices when a dependency is deleted explores the alternatives. At the moment the design is a bit asymmetric because for object_id I eagerly retract from mz_all_optimizer_notices, but for dependency_ids I use a SQL filter in the mz_optimizer_notices view definition. This is definitely something that I want to unify in the long term, currently leaning towards "eagerly remove entries" in both cases.

[ParkMyCar]

Personally I would love if we could unify the two, specifically if we could also eagerly retract from mz_all_optimizer_notices for the dependency_ids column.

[aalexandrov]

@ParkMyCar I guess since you are currently in the SQL Council I need your approval on this as well.

[ggnall]

This section looks good to me. For our current service model this also means Field Engineering can't look up prior notices and can only rely on asking for (or running) realtime EXPLAINs.

[aalexandrov]

This section looks good to me. For our current service model this also means Field Engineering can't look up prior notices and can only rely on asking for (or running) realtime EXPLAINs.

FYI several months ago we landed support for

EXPLAIN MATERIALIZED VIEW <item>
EXPLAIN INDEX <item>

which will the notices generated when we last planned <item>. In that sense the EXPLAINs are not realtime, but they still need to be executed ad-hoc by field eng. and there is no good way to get a sense of common notices occurring in many items.

[ggnall]

These defaults look good. And it's nice that a user can reveal a class of notice they've previously dismissed.

[ggnall]

This seems the most straightforward for an FE-targeted first version.

[ggnall]

Would notice maintenance be restricted from support users?

[aalexandrov]

While FE are the primary users of this feature we should find a way to enable it for them. Maybe once regular users starts seeing the notices this type of access should be revisited.

[ggevay]

Sorry, I got a bit lost here. As a general comment for this RBAC section, could you maybe structure the text in a way that the measures that we take (for each of the 3 options) and their consequences are somehow separated?

I don't really understand in what way 2. is any better than 1. They both say "All notices will be visible to all Materialize users.". I understand this as everybody sees everything. Does it matter whether it's only through the Console or SQL?

Notices can be only interpreted by superusers

What does "interpreted" mean here? Do I understand correctly that if we go with 3., then the notices view in the Console will be simply unusable only by ordinary users who are not superusers and support?

The UX will need to be built on top of a connection that uses the mz_support user.

How does the Console currently perform its other queries? Does it simply use the same user as the user who is viewing the Console (and mz_support if impersonated)? If yes, then do you mean we would introduce an exception in the Console code, where this specific thing would be performed on a different connection? Or you mean that we perform these queries only if we are the mz_support user?

The UX will hide notices if the current user is not either a superuser or a support user.

Is this sentence describing a measure that we take (i.e., guard the notice page in the Console code by an if that checks whether we are a superuser or a support user and show), or is this somehow a consequence of some other measures that we take?

The UX won't leak information by listing notices for catalog items that are not owned by the current user.

You mean that the Console will examine the RBAC attributes of each id in dependency_ids and/or object_id, and show only those notices for which RBAC says that it is owned by the current user? This will be the final solution in the long run, right? Btw. as an intermediate step before implementing this, could we make the Console show only the redacted columns to non-superusers?

[ggnall]

Not following why this must be the case? UX seems like the primary interface, but I'm not sure why SQL wouldn't be supported.

[aalexandrov]

I didn't consider that regular users where User::is_external_admin() is true will also be able to see the notices through the SQL console. I updated the text a bit to reflect that.

[ggevay]

A few more minor comments

[ggevay]

Is this the "creation" that also happens at bootstrap, or will this survive restarts? (I guess the latter would be more useful to users.)

[ggevay]

that that

[ggevay]

These are status updates by the user, right?

[ggevay]

So, do I understand correctly that mz_ignored_optimizer_notices needs to handle only the "ignored" state but is not concerned with the "resolved" state, because the "resolved" state doesn't need to survive restarts, because we expect those notices to simply not be recreated upon a restart?

[sthm]

Lgtm, thanks for working on this!

[sthm]

This seems an interesting followup, but it's fair to not consider it for the first release.

[ParkMyCar]

I love this idea! I need to understand a bit better how retractions from mz_all_optimizer_notices, would work. More than happy to chat in a huddle or zoom if that would help!

[ParkMyCar]

@aalexandrov I'm wondering if we can update the schema of mz_all_optimizer_notices to make the entries deterministic, and thus suitable for retractions? Do we need the created_at field or updated_at columns, and can we re-write message and hint to rely on object ids and not names?

[ParkMyCar]

Is it possible to make a single builtin source, mz_optimizer_notices instead of maintaining mz_all_optimizer_notices, mz_ignored_optimizer_notices, and then a mz_optimizer_notices view on top of the two?

Could mz_optimizer_notices be a builtin source, with the schema you describe above, but an additional state column, which is how we could mark certain notices as ignored?

[ParkMyCar]

Personally I would love if we could unify the two, specifically if we could also eagerly retract from mz_all_optimizer_notices for the dependency_ids column.

[ParkMyCar]

Can you go into more detail about how we would issue retractions? For example, if a dependent object was dropped, or an object was renamed, how do we issue a retraction, even by using the Catalog? The previous dependent ID is gone from the catalog, as is the previous name.

If there's already code for this in your PR that handles this scenario, I'm more than happy to read through that :)

[aalexandrov]

There are two ways we remove remove / supress notices from the mz_optimizer_notices view:

1. When an entry referenced in object_id is deleted, we remove the notice at the end of Catalog::transact.
2. When an entry referenced in dependency_ids is deleted, the current mz_optimizer_notices view definition will filter out the enclosing notice.

In the long term I would like to emit eager retractions also for (2), but for the the sake of merging an MVP that would allow me to put together an internal console app and expose the feature to field engineering for beta testing I thought this discrepancy won't hurt too much.

[ParkMyCar]

Do we have an estimate to how many entries might exist in mz_all_optimizer_notices? We should double check that filter pushdown works on a column, preferably on the object_id column which I'm guessing is what users will mostly filter on

[ParkMyCar]

Generally ALTER commands take the format ALTER <type> <name> <field> (e.g. alter cluster or alter index) what do you think about the following format?

ALTER NOTICE <fingerprint> SET STATUS = '<status>';
ALTER NOTICE <finterprint> RESET STATUS;

[ParkMyCar]

What about durably storing the notices, or some portion of the notice, in the Stash, instead of a builtin table/source? This is what we do for object comments, and it might alleviate some of the problems around retractions?

[aalexandrov]

What about durably storing the notices, or some portion of the notice, in the Stash, instead of a builtin table/source?

I think the main reason not to do that is that that optimizer notices are a function of the catalog state, more specifically a lot of them would be a function of properties of the plans for materialized views and indexes created during bootstrap.

This means that when we restart / upgrade materialize the plans might change. We would need to somehow discard all optimizer notices created before the restart and create new set of notice entries for the builtin source. Given these semantics, I thought that maintaining the optimizer notices in a builtin table is sufficient for now.

The only durable state that we really want to avoid losing between restart is the bit that defines whether the notice is ignored or not.

[aalexandrov]

I need to understand a bit better how retractions from mz_all_optimizer_notices, would work. More than happy to chat in a huddle or zoom if that would help!

Thanks for the review! I have an MVP open in #23360. I think it's best to have a huddle and go over the main parts of the PR together because I plan to change some things before I merge this PR and align the mechanisms used to retract invalid notices shortly after that with a follow-up PR.

[ParkMyCar]

@aalexandrov and I chatted over Zoom and we're aligned on this design!

For posterity, the follow ups we have for later versions of the feature are:

• Eagerly retracing notices when a dependency goes away, e.g. an index whose key was too wide gets dropped. We think this can be handled by adding a dependencies field to CatalogPlans, and then add a check to the Coordinator's consistency checks.
• Handling of the ignored/resolved state. This is a bit trickier because we need to survive restarts of envd, recreation of objects, and ideally renaming of the object, or the schema/database it exists in. There are some ideas on how to "fingerprint" this ignored/resolved state, but punting on it for now as it's not required for V0.

[aalexandrov]

@ParkMyCar I updated the design doc. If I can squeeze in the refactor to do the first bit (Eagerly retracing notices when a dependency goes away) I might tag you for a fast follow-up PR tomorrow.