[skunkwork] [adapter] Share allocators for all system / user id types#34296
Open
bkirwi wants to merge 6 commits intoMaterializeInc:mainfrom
Open
[skunkwork] [adapter] Share allocators for all system / user id types#34296bkirwi wants to merge 6 commits intoMaterializeInc:mainfrom
bkirwi wants to merge 6 commits intoMaterializeInc:mainfrom
Conversation
bkirwi
force-pushed
the
mzid
branch
14 times, most recently
from
5db9fdf to
b15df99
Compare
bkirwi
changed the title
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
1 participant
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.
Traditionally, different types of objects may end up with what appears to be the same ID: so the index
u1may be created on replicau1of clusteru1. This is I think generally considered to be confusing.This change switches to assigning IDs that are unique across different types - so we will avoid creating a new cluster
u2if an objectu2already exists, and vice-versa. This is done by giving ourselves a way to assign a new id that will not conflict with multiple different types, and adopting that incrementally.This does not guarantee that there are no ID collisions across types, especially because we do not change the IDs for any existing objects. (But we do avoid conflicts in many cases, as the test changes show.)
Motivation
First proposed in https://github.com/MaterializeInc/database-issues/issues/6336#issuecomment-2369825541.
I don't know if it has consensus as the right end state, but hopefully it is uncontroversial as an incremental step?
Tips for reviewer
One upshot of this is that Materialize will issue different IDs for things, including some built-in collections and clusters. (I did try and ensure that the system cluster IDs wouldn't change, since I suspect we have those hardcoded in some alerts and other places.) I think this is probably fine, but if you disagree, I am interested to hear about it!
I updated the regular CI tests, but there are probably some nightly failures that will crop up. I'll wait to resolve those until I think this is likely to be approved!
There are some other ID types that I haven't merged into the merged lists yet. I would prefer to save that for followup work, again assuming folks think this is a positive path.
An alternative to all this would be a catalog migration that collapses down all the different keys. This version is both more incremental and easier to reverse, but eventually something like that seems like it would make sense!