-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
schemachanger: support ALTER DATABASE .. CONFIGURE ZONE in DSC #121026
Conversation
Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
fcc6ca4
to
3cf6f42
Compare
3cf6f42
to
35eb624
Compare
35eb624
to
4ba3ae0
Compare
27cc8bf
to
c388c3d
Compare
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.
Reviewed 5 of 5 files at r1, 8 of 8 files at r2, 9 of 9 files at r3, 16 of 16 files at r4, 20 of 20 files at r5, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @annrpom)
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/configure_zone.go
line 497 at r5 (raw file):
) (zone *zonepb.ZoneConfig, seqNum uint32, err error) { if keys.RootNamespaceID == uint32(targetID) { zc, err := b.ZoneConfigGetter().GetZoneConfig(b, targetID)
Think we need to go through decomposition / resolve? Otherwise, are we in trouble if schema changes stack within a txn?
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/dependencies.go
line 82 at r3 (raw file):
// NodesStatusServer gives access to the NodesStatus service and is only // available when running as a system tenant. NodesStatusServer() *serverpb.OptionalNodesStatusServer
Nit: It feels like we should just pull in a narrower interface? WDYT?
pkg/sql/schemachanger/scdeps/exec_deps.go
line 237 at r4 (raw file):
var rawBytes []byte if oldZc != nil { rawBytes = oldZc.GetRawBytesInStorage()
Wait, shouldn't this be the raw bytes for the new zone config?
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.
I'm reviewing this to better understand the DSC. Some of my comments are just questions about the code.
Reviewed 5 of 5 files at r1, 8 of 8 files at r2, 9 of 9 files at r3, 16 of 16 files at r4.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @annrpom)
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/configure_zone.go
line 48 at r5 (raw file):
} // TODO(annie): implement complete support for CONFIGURE ZONE
Can you be more specific about what this TODO means? Does it mean we want to support CONFIGURE ZONE for things other than the database?
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/configure_zone.go
line 54 at r5 (raw file):
// Block from using YAML config unless we are discarding a YAML config. if n.YAMLConfig != nil && !n.Discard { panic(scerrors.NotImplementedErrorf(n, "YAML config is deprecated and not supported in DSC"))
Is DSC a term that a customer would know? I wonder if we should have a better error message that an end user may understand.
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/configure_zone.go
line 83 at r5 (raw file):
panic(err) } if elem.DatabaseID == 104 {
Is this intentional or something left over from test?
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/configure_zone.go
line 96 at r5 (raw file):
// For the system database, the user must be an admin. Otherwise, we // require CREATE or ZONECONFIG privilege on the database in question. reqNonAdminPrivs := []privilege.Kind{privilege.ZONECONFIG, privilege.CREATE}
nit: suggest moving this declaration down to when we need it. It's only needed in the error case, so make sense to declare right before returning the error
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/configure_zone.go
line 302 at r5 (raw file):
} // TODO Guard against system tenant here
If we are missing guards, is it possible to do unauthorized modifications to the system tenant? Just trying to understand what happens if we don't address this right away.
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.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @fqazi and @spilchen)
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/configure_zone.go
line 48 at r5 (raw file):
Previously, spilchen wrote…
Can you be more specific about what this TODO means? Does it mean we want to support CONFIGURE ZONE for things other than the database?
done. yes! although getting database out in this PR gets a chunk of the work done -- the other PRs that i will stop hoarding (like #125786)
the list of objects we surpport configure zone on is: https://www.cockroachlabs.com/docs/stable/configure-replication-zones
i'll add a list in to be more verbose
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/configure_zone.go
line 54 at r5 (raw file):
Previously, spilchen wrote…
Is DSC a term that a customer would know? I wonder if we should have a better error message that an end user may understand.
done. i think a similar comment was left on Bergin's comment on type PR -- i'll spell this out to "declarative schema changer" since we do actually document the declarative schema changer (https://www.cockroachlabs.com/docs/stable/online-schema-changes#declarative-schema-changer), but "DSC" is a bit less obvious
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/configure_zone.go
line 83 at r5 (raw file):
Previously, spilchen wrote…
Is this intentional or something left over from test?
done -- left over! thx for catching
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/configure_zone.go
line 96 at r5 (raw file):
Previously, spilchen wrote…
nit: suggest moving this declaration down to when we need it. It's only needed in the error case, so make sense to declare right before returning the error
done -- makes sense in terms of readability ('-')7
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/configure_zone.go
line 302 at r5 (raw file):
Previously, spilchen wrote…
If we are missing guards, is it possible to do unauthorized modifications to the system tenant? Just trying to understand what happens if we don't address this right away.
done. i'll freshen up this TODO -- for more context, we need to guard against non-system/secondary tenants setting zone configs on any system ranges (https://www.cockroachlabs.com/docs/stable/configure-replication-zones#create-a-replication-zone-for-a-system-range). this is inspired by
cockroach/pkg/sql/set_zone_config.go
Lines 414 to 425 in 9102419
// Secondary tenants are not allowed to set zone configurations on any named | |
// zones other than RANGE DEFAULT. | |
if !params.p.execCfg.Codec.ForSystemTenant() { | |
zoneName, found := zonepb.NamedZonesByID[uint32(targetID)] | |
if found && zoneName != zonepb.DefaultZoneName { | |
return pgerror.Newf( | |
pgcode.CheckViolation, | |
"non-system tenants cannot configure zone for %s range", | |
zoneName, | |
) | |
} | |
} |
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/configure_zone.go
line 497 at r5 (raw file):
Previously, fqazi (Faizan Qazi) wrote…
Think we need to go through decomposition / resolve? Otherwise, are we in trouble if schema changes stack within a txn?
may i get an example/clarification of what you are talking about? does it mean for a situation like the following:
demo@127.0.0.1:26257/demoapp/movr> set use_declarative_schema_changer =
-> unsafe_always;
SET
Time: 1ms total (execution 1ms / network 0ms)
demo@127.0.0.1:26257/demoapp/movr> begin;
BEGIN
Time: 0ms total (execution 0ms / network 0ms)
demo@127.0.0.1:26257/demoapp/movr OPEN> ALTER DATABASE annie CONFIGURE ZONE
-> USING num_replicas = 19;
CONFIGURE ZONE 0
Time: 6ms total (execution 6ms / network 0ms)
demo@127.0.0.1:26257/demoapp/movr OPEN> show zone configuration for database
-> annie;
target | raw_config_sql
-----------------+--------------------------------------------
DATABASE annie | ALTER DATABASE annie CONFIGURE ZONE USING
| range_min_bytes = 134217728,
| range_max_bytes = 536870912,
| gc.ttlseconds = 14400,
| num_replicas = 19,
| constraints = '[]',
| lease_preferences = '[]'
(1 row)
Time: 1ms total (execution 1ms / network 0ms)
demo@127.0.0.1:26257/demoapp/movr OPEN> ALTER DATABASE annie CONFIGURE ZONE
-> USING num_replicas = 18;
CONFIGURE ZONE 0
Time: 5ms total (execution 4ms / network 0ms)
demo@127.0.0.1:26257/demoapp/movr OPEN> show zone configuration for database
-> annie;
target | raw_config_sql
-----------------+--------------------------------------------
DATABASE annie | ALTER DATABASE annie CONFIGURE ZONE USING
| range_min_bytes = 134217728,
| range_max_bytes = 536870912,
| gc.ttlseconds = 14400,
| num_replicas = 18,
| constraints = '[]',
| lease_preferences = '[]'
(1 row)
Time: 1ms total (execution 1ms / network 0ms)
-- just checking that we are indeed in dsc-land
demo@127.0.0.1:26257/demoapp/movr OPEN> explain (ddl) ALTER DATABASE annie
-> CONFIGURE ZONE USING num_replicas = 18;
info
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Schema change plan for ALTER DATABASE ‹annie› CONFIGURE ZONE USING ‹num_replicas› = ‹18›; following ALTER DATABASE ‹annie› CONFIGURE ZONE USING ‹num_replicas› = ‹19›; ALTER DATABASE ‹annie› CONFIGURE ZONE USING ‹num_replicas› = ‹18›;
├── StatementPhase
│ └── Stage 1 of 1 in StatementPhase
│ ├── 1 element transitioning toward PUBLIC
│ │ └── ABSENT → PUBLIC DatabaseZoneConfig:{DescID: 114 (annie), SeqNum: 3}
│ └── 1 Mutation operation
│ └── AddDatabaseZoneConfig {"DatabaseID":114,"SeqNum":3}
└── PreCommitPhase
├── Stage 1 of 2 in PreCommitPhase
│ ├── 3 elements transitioning toward PUBLIC
│ │ ├── PUBLIC → ABSENT DatabaseZoneConfig:{DescID: 114 (annie), SeqNum: 1}
│ │ ├── PUBLIC → ABSENT DatabaseZoneConfig:{DescID: 114 (annie), SeqNum: 2}
│ │ └── PUBLIC → ABSENT DatabaseZoneConfig:{DescID: 114 (annie), SeqNum: 3}
│ └── 1 Mutation operation
│ └── UndoAllInTxnImmediateMutationOpSideEffects
└── Stage 2 of 2 in PreCommitPhase
├── 3 elements transitioning toward PUBLIC
│ ├── ABSENT → PUBLIC DatabaseZoneConfig:{DescID: 114 (annie), SeqNum: 1}
│ ├── ABSENT → PUBLIC DatabaseZoneConfig:{DescID: 114 (annie), SeqNum: 2}
│ └── ABSENT → PUBLIC DatabaseZoneConfig:{DescID: 114 (annie), SeqNum: 3}
└── 3 Mutation operations
├── AddDatabaseZoneConfig {"DatabaseID":114,"SeqNum":1}
├── AddDatabaseZoneConfig {"DatabaseID":114,"SeqNum":2}
└── AddDatabaseZoneConfig {"DatabaseID":114,"SeqNum":3}
(1 row)
Time: 4ms total (execution 4ms / network 0ms)
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/dependencies.go
line 82 at r3 (raw file):
Previously, fqazi (Faizan Qazi) wrote…
Nit: It feels like we should just pull in a narrower interface? WDYT?
done -- yeah, i think that i just slapped this in here because it sounded remotely similar, but i don't think similar enough to not warrant another interface
pkg/sql/schemachanger/scdeps/exec_deps.go
line 237 at r4 (raw file):
Previously, fqazi (Faizan Qazi) wrote…
Wait, shouldn't this be the raw bytes for the new zone config?
let me add a comment or something, but it seems like we either update or insert whenever we are writing zone config to batch where:
- an insert is denoted by a value needing to be written (the ZoneConfigWithRawBytes's
zc
) and an empty expected values (since we don't expect any values to be in storage for a fresh insert) - an upsert is denoted by a value needing to be written (the ZoneConfigWithRawBytes's
zc
) and expected values for the values we are overwriting (ZoneConfigWithRawBytes'srb
)
i took a look at the other code that calls on WriteZoneConfigToBatch and they seem to follow the same protocol (where zc and rb are not the same) like in repair.go:
Lines 836 to 853 in 48ebfd1
zc, err := p.Descriptors().GetZoneConfig(ctx, p.txn, tbl.GetID()) | |
if err != nil { | |
return err | |
} | |
if zc == nil { | |
zc = zone.NewZoneConfigWithRawBytes(&zonepb.ZoneConfig{}, nil /* expected raw bytes */) | |
} else { | |
zc = zc.Clone() | |
} | |
if gc := zc.ZoneConfigProto().GC; gc == nil { | |
zc.ZoneConfigProto().GC = &zonepb.GCPolicy{} | |
} | |
zc.ZoneConfigProto().GC.TTLSeconds = int32(ttl.Nanos() / int64(time.Second)) | |
// Write the new or updated zone config. | |
b := p.txn.NewBatch() | |
if err := p.Descriptors().WriteZoneConfigToBatch( | |
ctx, p.extendedEvalCtx.Tracing.KVTracingEnabled(), b, tbl.GetID(), zc, |
c388c3d
to
f2ce69a
Compare
f2ce69a
to
f10bdd4
Compare
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.
Great work, I just have a few minor comments. Its good to merge after.
Reviewed 48 of 48 files at r6, 15 of 15 files at r7, 21 of 21 files at r8, 25 of 25 files at r9, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @annrpom and @spilchen)
pkg/sql/descriptor.go
line 296 at r7 (raw file):
provider := p.regionsProvider() if provider == nil { return errors.New("no regions provider available")
errors.AssertionFailed? I don't think we expect this ever?
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/configure_zone.go
line 497 at r5 (raw file):
-> USING num_replicas = 18;
Can you try changing gc.ttlseconds and num_replicas in the same txn? Just want to make sure we have both changes by the end of the txn.
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/configure_zone.go
line 61 at r9 (raw file):
// Block from using YAML config unless we are discarding a YAML config. if n.YAMLConfig != nil && !n.Discard { panic(scerrors.NotImplementedErrorf(n,
Just as a a follow on maybe we should toss a warning on YAML in legacy to make this clear, if we don't have one.
pkg/sql/schemachanger/scdeps/exec_deps.go
line 237 at r4 (raw file):
Previously, annrpom (annie pompa) wrote…
let me add a comment or something, but it seems like we either update or insert whenever we are writing zone config to batch where:
- an insert is denoted by a value needing to be written (the ZoneConfigWithRawBytes's
zc
) and an empty expected values (since we don't expect any values to be in storage for a fresh insert)- an upsert is denoted by a value needing to be written (the ZoneConfigWithRawBytes's
zc
) and expected values for the values we are overwriting (ZoneConfigWithRawBytes'srb
)i took a look at the other code that calls on WriteZoneConfigToBatch and they seem to follow the same protocol (where zc and rb are not the same) like in repair.go:
Lines 836 to 853 in 48ebfd1
zc, err := p.Descriptors().GetZoneConfig(ctx, p.txn, tbl.GetID()) if err != nil { return err } if zc == nil { zc = zone.NewZoneConfigWithRawBytes(&zonepb.ZoneConfig{}, nil /* expected raw bytes */) } else { zc = zc.Clone() } if gc := zc.ZoneConfigProto().GC; gc == nil { zc.ZoneConfigProto().GC = &zonepb.GCPolicy{} } zc.ZoneConfigProto().GC.TTLSeconds = int32(ttl.Nanos() / int64(time.Second)) // Write the new or updated zone config. b := p.txn.NewBatch() if err := p.Descriptors().WriteZoneConfigToBatch( ctx, p.extendedEvalCtx.Tracing.KVTracingEnabled(), b, tbl.GetID(), zc,
Oh the name is just wacky and the comment on the zone config side isn't great:
// ZoneConfigWithRawBytes wraps a zone config together with its expected raw bytes. For cached modified zone configs, the raw bytes should be the deserialized bytes from the zone config proto. Though, the raw bytes should be the raw value read from kv when it's first read from storage.
^ The second part isn't clear. Probably should be clearer and say what we expect when doing a CPut
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.
Thanks for addressing my earlier comments. I just had a follow-up question.
Reviewed 48 of 48 files at r6, 15 of 15 files at r7, 21 of 21 files at r8.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @annrpom)
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/configure_zone.go
line 48 at r5 (raw file):
Previously, annrpom (annie pompa) wrote…
done. yes! although getting database out in this PR gets a chunk of the work done -- the other PRs that i will stop hoarding (like #125786)
the list of objects we surpport configure zone on is: https://www.cockroachlabs.com/docs/stable/configure-replication-zones
i'll add a list in to be more verbose
Is the support for configure zone on a table in a separate codepath? Maybe I am missing something, but it looks like the support in this file is just for the database object.
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/configure_zone.go
line 155 at r9 (raw file):
if options == nil { // User is trying to update a zone config value that's protected for // multi-region databases. Return the constructed error.
nit: I wasn't sure if this was copied from below, where it does make sense.
Suggestion:
// User is trying to remove the protected multi-region zone config.
// Return the constructed error.
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.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @fqazi and @spilchen)
pkg/sql/descriptor.go
line 296 at r7 (raw file):
Previously, fqazi (Faizan Qazi) wrote…
errors.AssertionFailed? I don't think we expect this ever?
done
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/configure_zone.go
line 48 at r5 (raw file):
it looks like the support in this file is just for the database object
for this PR, yes -- i realized this question is probably centered around the fact that i mistakenly put table in my comment. fixed!
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/configure_zone.go
line 497 at r5 (raw file):
Previously, fqazi (Faizan Qazi) wrote…
-> USING num_replicas = 18;
Can you try changing gc.ttlseconds and num_replicas in the same txn? Just want to make sure we have both changes by the end of the txn.
demo@127.0.0.1:26257/demoapp/movr> set use_declarative_schema_changer = unsafe_always;
SET
Time: 1ms total (execution 1ms / network 0ms)
demo@127.0.0.1:26257/demoapp/movr> begin;
BEGIN
Time: 0ms total (execution 0ms / network 0ms)
demo@127.0.0.1:26257/demoapp/movr OPEN> alter database annie configure zone using num_replicas = 4;
CONFIGURE ZONE 0
Time: 3ms total (execution 3ms / network 0ms)
demo@127.0.0.1:26257/demoapp/movr OPEN> alter database annie configure zone using gc.ttlseconds = 13300;
CONFIGURE ZONE 0
Time: 3ms total (execution 3ms / network 0ms)
demo@127.0.0.1:26257/demoapp/movr OPEN> show zone configuration for database annie;
target | raw_config_sql
-----------------+--------------------------------------------
DATABASE annie | ALTER DATABASE annie CONFIGURE ZONE USING
| range_min_bytes = 134217728,
| range_max_bytes = 536870912,
| gc.ttlseconds = 13300,
| num_replicas = 4,
| constraints = '[]',
| lease_preferences = '[]'
(1 row)
Time: 2ms total (execution 1ms / network 0ms)
demo@127.0.0.1:26257/demoapp/movr OPEN> commit;
COMMIT
Time: 11ms total (execution 8ms / network 3ms)
demo@127.0.0.1:26257/demoapp/movr> show zone configuration for database annie;
target | raw_config_sql
-----------------+--------------------------------------------
DATABASE annie | ALTER DATABASE annie CONFIGURE ZONE USING
| range_min_bytes = 134217728,
| range_max_bytes = 536870912,
| gc.ttlseconds = 13300,
| num_replicas = 4,
| constraints = '[]',
| lease_preferences = '[]'
(1 row)
Time: 6ms total (execution 5ms / network 0ms)
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/configure_zone.go
line 61 at r9 (raw file):
Previously, fqazi (Faizan Qazi) wrote…
Just as a a follow on maybe we should toss a warning on YAML in legacy to make this clear, if we don't have one.
sure -- let me clarify what makes sense with dikshant and then add that in a smaller follow-up pr. i recall him suggesting a cluster setting at one point...
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/configure_zone.go
line 155 at r9 (raw file):
Previously, spilchen wrote…
nit: I wasn't sure if this was copied from below, where it does make sense.
thanks for the catch! i removed this part since it is guarded by the fact that we fallback to legacy when the user is trying to discard zone config
pkg/sql/schemachanger/scdeps/exec_deps.go
line 237 at r4 (raw file):
Previously, fqazi (Faizan Qazi) wrote…
Oh the name is just wacky and the comment on the zone config side isn't great:
// ZoneConfigWithRawBytes wraps a zone config together with its expected raw bytes. For cached modified zone configs, the raw bytes should be the deserialized bytes from the zone config proto. Though, the raw bytes should be the raw value read from kv when it's first read from storage.
^ The second part isn't clear. Probably should be clearer and say what we expect when doing a CPut
done -- tried to make it a bit clearer
f10bdd4
to
71de385
Compare
71de385
to
0f73b0d
Compare
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.
lgtm
Reviewed 53 of 53 files at r10, 8 of 8 files at r11, 10 of 10 files at r12, 18 of 18 files at r13, 20 of 20 files at r14, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @fqazi)
61f553b
to
af519d7
Compare
af519d7
to
78bcbbd
Compare
44b30b5
to
1583af7
Compare
This commit introduces the DatabaseZoneConfig element that will become relevant for supporting `ALTER DATABASE ... CONFIGURE ZONE` in DSC. Release note: None
52e1027
to
357c0f0
Compare
This patch adds the necessary interfaces/functions for zone config verification and retrieval. Informs: cockroachdb#117574 Release note: None
This patch adds the functionality to configure a zone configuration on a database. Informs: cockroachdb#117574 Release note: None
357c0f0
to
bd638f2
Compare
TFTR! ('-')7 bors r=fqazi, spilchen |
scpb: Introduce DatabaseZoneConfig element
This commit introduces the DatabaseZoneConfig element that will become
relevant for supporting
ALTER DATABASE ... CONFIGURE ZONE
in DSC.schemachanger: add db zone config opgen rule
This patch adds an opgen rule for db zone configs
in the ADD direction, along with some interfaces
for zone config verification and retrieval.
schemachanger: support DB zone config
This patch adds the functionality to configure
a zone configuration on a database.
Informs: #117574
Epic: CRDB-35252
Release note: None
Fixes: #117574