sql/catalog/descs,multiregionccl: allow system database to be made MR#91970
Conversation
|
This change is |
|
@jeffswenson you're going to need this |
50da5be to
76b2475
Compare
|
Thanks! I'm going to cherry pick this into my sql_instances change so that I can send it out for review. |
postamar
left a comment
There was a problem hiding this comment.
LGTM. I think you took care of all of the gotchas that made the system database special. At least, I can't think of any more.
pkg/sql/catalog/descs/hydrate.go
Outdated
| } | ||
| mutableLookupFunc := func(ctx context.Context, id descpb.ID) (catalog.Descriptor, error) { | ||
| // This special case exists to deal with the desire to use enums in the | ||
| // system, and the fact that the hydration contract is such that when we |
There was a problem hiding this comment.
nit: did you mean "in the system database"?
| Required: true, | ||
| AvoidLeased: true, | ||
| }) | ||
| } |
There was a problem hiding this comment.
I worry that, whenever we do endow system.public with an actual descriptor, removing this code will be overlooked. I'm not sure what to do about this, though. Perhaps me referencing #89849 is enough.
|
|
||
| // This should only every happen to the system database. Unlike many other | ||
| // post-deserialization changes, this does not need to last forever to | ||
| // support restores. It can be removed in 23.2. |
There was a problem hiding this comment.
It can be removed in 23.2.
Why is that the case?
|
|
||
| // This is a rather contrived test to demonstrate that one can successfully, | ||
| // albeit with some arm-twisting, convince the sql layer to make regional- | ||
| // by-row tables in the system database. |
There was a problem hiding this comment.
Shouldn't this PR include a new CCL logic test config in which the system database is multi-region? Seems like it'd be worth doing because it'd be nice to add more test cases, then again I have no idea what kind of effort it involves. You be the judge.
…ypes This change is needed in order to convert system database tables to regional by row. Release note: None
This PR removes some logic which prevented the system database from being properly modified. It also adds a test which ensures that the changes happen as expected. Release note: None
There are very old versions out there which do not have versions in their descriptors. Long ago when these versions were introduced, we incremented the version on change. The only descriptor we never actually wrote to was the system descriptor. Those days are over. To account for that, we set its version to 1 implicitly during post-deserialization. This fixes the version upgrade tests. Release note: None
76b2475 to
c61238b
Compare
ajwerner
left a comment
There was a problem hiding this comment.
TFTR!
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @postamar)
pkg/sql/catalog/dbdesc/database_desc_builder.go line 112 at r3 (raw file):
Previously, postamar (Marius Posta) wrote…
It can be removed in 23.2.
Why is that the case?
When I typed this, my thinking was that we'd have a migration in 23.1 which upgrades the system database. Nothing right now ensures that. I've reworded this comment.
pkg/sql/catalog/descs/hydrate.go line 125 at r3 (raw file):
Previously, postamar (Marius Posta) wrote…
nit: did you mean "in the system database"?
Yes, done.
pkg/sql/catalog/descs/hydrate.go line 136 at r3 (raw file):
Previously, postamar (Marius Posta) wrote…
I worry that, whenever we do endow system.public with an actual descriptor, removing this code will be overlooked. I'm not sure what to do about this, though. Perhaps me referencing #89849 is enough.
Probably it is good enough. The good news is it's still legit in that because the only thing we would need to see out of a mutable descriptor for this descriptor might be a changed name, and I don't think we'll permit changing the name of the public schema (we already don't).
pkg/ccl/multiregionccl/regional_by_row_system_database_test.go line 24 at r3 (raw file):
Previously, postamar (Marius Posta) wrote…
Shouldn't this PR include a new CCL logic test config in which the system database is multi-region? Seems like it'd be worth doing because it'd be nice to add more test cases, then again I have no idea what kind of effort it involves. You be the judge.
I think such a thing will be good. I'll add it in a later PR. Filed #92090.
|
bors r+ |
|
Build succeeded: |
This change is needed in order to convert system database tables to
regional by row.
This PR removes some logic which prevented the system database from being
properly modified. It also adds a test which ensures that the changes happen
as expected.
Epic: CRDB-18596
Release note: None