Skip to content
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

Jump to bottom

adapter: move introspection relations to mz_introspection #27830

Merged
merged 4 commits into from
Jun 27, 2024
Merged

adapter: move introspection relations to mz_introspection #27830

merged 4 commits into from
Jun 27, 2024

Conversation

teskje
Copy link
Contributor

@teskje teskje commented Jun 24, 2024 •
edited
Loading

This PR moves all per-replica introspection relations into their own system schema, mz_introspection.

The change has two major benefits:

  • Relations that contain replica-specific data, and therefore require replica-targeting to be queried, are now recognizable from the schema they reside in.
  • New unified introspection relations can use the same names as existing non-unified ones. Conflicts are avoided due to them residing in different schemas.

This PR includes a compatibility mechanism that looks up queried relations in mz_introspection (and mz_catalog_unstable) if they are not found in mz_internal. This is mostly meant to simplify the migration of our own internal clients. External users also benefit from it, but there is no current plan to maintain the compatibility mechanism for a long time.

Motivation

  • This PR adds a known-desirable feature.

Part of https://github.com/MaterializeInc/database-issues/issues/7110

Tips for reviewer

The diff is of course huge, but mostly just replacing the schema names, so I hope it's pretty skimable.

The commits that I think require closest review are commit (3), which introduces the compatibility mechanism, and commit (4), which updates the user documentation.

Checklist

  • This PR has adequate test coverage / QA involvement has been duly considered. (trigger-ci for additional test/nightly runs)
  • 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:
    • Move the replica introspection relations from the mz_internal schema into a new mz_introspection schema.

@nrainer-materialize nrainer-materialize changed the title adapter: move introspection relations to mz_introspeciton adapter: move introspection relations to mz_introspection Jun 24, 2024
@teskje teskje force-pushed the move-unstable-relations branch 4 times, most recently from 2c2b743 to 66ca7ad Compare June 25, 2024 13:28
@teskje teskje marked this pull request as ready for review June 25, 2024 17:00
@teskje teskje requested review from a team and morsapaes as code owners June 25, 2024 17:00
@teskje teskje requested a review from maddyblue June 25, 2024 17:00
maddyblue
maddyblue approved these changes Jun 25, 2024
View reviewed changes
Copy link
Contributor

@maddyblue maddyblue left a comment

Choose a reason for hiding this comment

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

Adapter parts lgtm.

src/adapter/src/catalog/state.rs
);

if let Some(id) = get_schema_entries(schema).get(&name.item) {
debug!(
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be a metric instead of a debug that defaults to off? As a metric we'd be able to easily have certainty when we could safely remove this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Metrics are unfortunately not available in this context (CatalogState). We can add them, but I'd rather to that in a different PR to keep the scope of this one small.

morsapaes
morsapaes approved these changes Jun 26, 2024
View reviewed changes
Copy link
Contributor

@morsapaes morsapaes left a comment

Choose a reason for hiding this comment

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

The documentation changes look good to me. Probably a misunderstanding on my part, but I was expecting to be able to use both mz_internal and mz_catalog_unstable to resolve relations that weren't migrated to mz_introspection, but will eventually be migrated off mz_internal; what is the plan to start falling back to the new schema name?

@teskje
Copy link
Contributor Author

teskje commented Jun 26, 2024

The documentation changes look good to me. Probably a misunderstanding on my part, but I was expecting to be able to use both mz_internal and mz_catalog_unstable to resolve relations that weren't migrated to mz_introspection, but will eventually be migrated off mz_internal; what is the plan to start falling back to the new schema name?

This PR introduces the translation from mz_internal to both mz_catalog_unstable and mz_introspection. However, there are no relations in mz_catalog_unstable yet, so the former translation isn't used. I limited myself to moving things into mz_introspection here to keep this diff as small as possible. In a follow-up PR I'll move the remaining relations from mz_internal to mz_catalog_unstable and also create the documentation page for that new schema (I figured it would just be confusing to have one for an empty schema).

@teskje
adapter: move introspection sources into mz_introspection
0b380ad 
This commit moves all per-replica introspection sources, as well as
objects depending on them, into the new `mz_introspection` schema. It
also adds a test to ensure that `mz_introspection` contains all objects
that depend on introspection sources, and doesn't contain any others.
@teskje
test: adapt to new schema of introspection relations
1135ff9
@teskje
adapter: compatibility translation for mz_internal schema
d372555 
This commit adds a compatibility mechanism that translates uses of
relations that have been moved to `mz_cluster_unstable` or
`mz_introspection` from `mz_internal` automatically if they are
referenced through the old schema. This is to simplify the migration
effort.
@teskje
doc/user: update schema of introspection relations
5d4bdd3 
This commit updates the schema of replica introspection relations from
`mz_internal` to `mz_introspection`. As part of this, a new page for the
`mz_introspection` system catalog schema is created and added to the
list of unstable schemas.
@teskje
Copy link
Contributor Author

teskje commented Jun 27, 2024

TFTRs!

@teskje teskje merged commit 6f20b2c into MaterializeInc:main Jun 27, 2024
81 checks passed
@teskje teskje deleted the move-unstable-relations branch June 27, 2024 11:40
@teskje teskje mentioned this pull request Jul 8, 2024
Draft
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maddyblue maddyblue maddyblue approved these changes

@morsapaes morsapaes morsapaes approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@teskje @maddyblue @morsapaes