-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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.
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
sql: alter ALTER TYPE ... OWNER TO ... for multi-region enum #69722
Conversation
Release justification: fix for old functionality Release note (sql change): Previously, one could not alter the owner of the crdb_internal_region type which is created by initiating a multi-region database. This is now possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code change in itself looks good.
I'm curious to hear your thoughts about the behaviour here -- what do you think about this change vs. making it such that the database owner, for multi-region databases, is always the owner of the crdb_internal_region
type (and it's corresponding array type descriptor)? Do we care if these two diverge?
Reviewable status:
complete! 0 of 0 LGTMs obtained
Given that certain perms are tied to the multi region enum and not the db I think ownership should not be the same. |
So should we instead change the multi-region enum's owner when the database owner changes? |
i don't think so, since we treat db/type permissions different right now. enums iiuc also have different permissions so i think they should be different. |
i said the wrong thing above ^ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a fair point -- looks don't block GRANT/REVOKE on crdb_internal_regions
. I'm not sure if this was by design or an oversight, but I guess changing ownership on the type should be allowed if GRANT/REVOKE are.
Reviewable status:
complete! 0 of 0 LGTMs obtained
bors r=arulajmani :D |
Build failed: |
bors r=arulajmani |
Build succeeded: |
Release justification: fix for old functionality
Resolves #69714
Release note (sql change): Previously, one could not alter the owner of
the crdb_internal_region type which is created by initiating a
multi-region database. This is now possible.