sql: allow ALTER DATABASE PRIMARY REGION to work on system tenants #121293
Conversation
jasminejsun
requested review from
DarrylWong and
renatolabs
and removed request for
a team
|
This change is |
jasminejsun
force-pushed
the
mr_system_database
branch
from
6de1305 to
9002107
Compare
jasminejsun
force-pushed
the
mr_system_database
branch
from
9002107 to
771e97b
Compare
jasminejsun
force-pushed
the
mr_system_database
branch
2 times, most recently
from
5463cae to
ef0fde6
Compare
There was a problem hiding this comment.
Reviewed 2 of 6 files at r1, 1 of 1 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @annrpom and @jasminejsun)
pkg/cmd/roachtest/tests/multi_region_system_database.go line 117 at r3 (raw file):
t.WorkerStatus("stop") stopOpts := option.DefaultStopOpts() c.Stop(ctx, t.L(), stopOpts, c.Node(i))
I think this does a graceful kill, lets be a bit more aggressive:
c.Run(ctx, install.WithNodes(c.Node(i), "killall -9 cockroach")
pkg/ccl/multiregionccl/multiregion_system_table_test.go line 310 at r3 (raw file):
} func TestMRSystemDatabase(t *testing.T) {
I'm thinking we just modify the multi-tenant multi-region one above to also test the system tenant. What do you think?
jasminejsun
force-pushed
the
mr_system_database
branch
2 times, most recently
from
3fa7ec1 to
cd7939e
Compare
There was a problem hiding this comment.
Reviewed 1 of 2 files at r4.
Reviewable status:
pkg/ccl/multiregionccl/multiregion_system_table_test.go line 84 at r4 (raw file):
tDB.CheckQueryResults(t, `SELECT * FROM crdb_internal.invalid_objects`, [][]string{}) _, err = sqlDB.Exec(`SET CLUSTER SETTING sql.multiregion.preview_multiregion_system_database.enabled = true`)
Okay perfect, now lets just modify the tests below to check with both tDB and sqlDB. We can use a loop to pick one of the databases and confirm the same properties hold.
jasminejsun
force-pushed
the
mr_system_database
branch
from
cd7939e to
22484cb
Compare
Previously, on a multi-region setup the system database could not be modified to be multi-region and it was blocked from being made multi-region aware. To address this, we are now allowing ALTER DATABASE PRIMARY REGION to work on system tenants. Fixes: cockroachdb#63365 Epic: CRDB-33032 Release note (sql change): Previously, we added support for settings reegion on the system database, which was limited to tenants only. We lifted this limitation to allow ALTER DATABASE PRIMARY REGION to work on system tenants. To support preview status, we created a cluster setting called sql.multiregion.preview_multiregion_system_database that will give users the option to set up their system database as multi-region for Cockroach dedicated (this cluster setting is not enabled by default). Note that after adding non-primary regions, we recommend that users do a rolling restart to propogate region information.
jasminejsun
force-pushed
the
mr_system_database
branch
from
22484cb to
6f3cae0
Compare
There was a problem hiding this comment.
Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status:
pkg/ccl/multiregionccl/multiregion_system_table_test.go line 91 at r5 (raw file):
sDB.Exec(t, `ALTER DATABASE system ADD REGION "us-east3"`) testCases := []struct {
Nice!
|
bors r+ |
Previously, on a multi-region setup the system database could not be modified to be multi-region and it was blocked from being made multi-region aware. To address this, we are now allowing ALTER DATABASE PRIMARY REGION to work on system tenants.
Fixes: #63365
Epic: CRDB-33032
Release note (sql change): Previously, we added support for settings reegion on the system database, which was limited to tenants only. We lifted this limitation to allow ALTER DATABASE PRIMARY REGION to work on system tenants. Note that after adding non-primary regions, we recommend that users do a rolling restart to propogate region information.