schemachanger: add support for discarding subzones #134826
Conversation
![blathers[crl]](https://avatars.githubusercontent.com/in/59700?s=40&u=373d9101a12edc3847d2fb6ae78e3adcfb4ca41e&v=4){kind=avatar}
|
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. |
|
This change is |
annrpom
force-pushed
the
dsc-discard-subzones
branch
3 times, most recently
from
337b29d
to
6444512
Compare
annrpom
force-pushed
the
dsc-discard-subzones
branch
10 times, most recently
from
09535d3
to
aa1f86e
Compare
|
There's a lot of generated tests here and I think the most valuable thing we get from them (the explain plans) is ensuring that seqNum is tracked correctly for explicit txns. Maybe it would make more sense to just convert these to logictests that test the correctness instead since those can fail if seqNums aren't tracked correctly anyways... thoughts? |
There was a problem hiding this comment.
Great work @annrpom, I had some very minor comments.
Reviewed 26 of 26 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @annrpom)
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/database_zone_config.go line 49 at r1 (raw file):
) ([]scpb.Element, []scpb.Element) { var elems []scpb.Element if dzo.seqNum > 0 {
Maybe a comment for clarity, but I think this says that all past IDs are dropped. Could we just adjust the loop to do this for us...
for i :=0; i < max(dzo.seqNum), 1), ;i++
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/named_range_zone_config.go line 47 at r1 (raw file):
_ BuildCtx, ) ([]scpb.Element, []scpb.Element) { var elems []scpb.Element
Similar comment here
annrpom
force-pushed
the
dsc-discard-subzones
branch
from
aa1f86e
to
6ce0555
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/database_zone_config.go line 49 at r1 (raw file):
Previously, fqazi (Faizan Qazi) wrote…
Maybe a comment for clarity, but I think this says that all past IDs are dropped. Could we just adjust the loop to do this for us...
for i :=0; i < max(dzo.seqNum), 1), ;i++
done -- i ended up using a b.QueryByID(dzo.getTargetID()).FilterDatabaseZoneConfig().ForEach
pkg/sql/schemachanger/scbuild/internal/scbuildstmt/named_range_zone_config.go line 47 at r1 (raw file):
Previously, fqazi (Faizan Qazi) wrote…
Similar comment here
done -- similar to above
annrpom
force-pushed
the
dsc-discard-subzones
branch
2 times, most recently
from
9d8f042
to
3e19b8e
Compare
Previously, discarding a zone config on an object in an explicit transaction would only discard the latest (categorized by highest seq num) element associated with it. This would mean for the following scenario: ``` BEGIN; ALTER TABLE t CONFIGURE ZONE USING num_replicas = 11; ALTER TABLE t CONFIGURE ZONE USING num_replicas = 12; ALTER TABLE t CONFIGURE ZONE DISCARD; COMMIT; ``` The discard would only discard the element with a replication factor of 12, leaving us with a table with a rep. factor of 11. This patch ensures we clean up all references of an element when discarding. Epic: none Release note: None
This patch adds support for discarding a subzone config in the DSC. Fixes: cockroachdb#133158 Epic: none Release note: None
annrpom
force-pushed
the
dsc-discard-subzones
branch
from
3e19b8e
to
bd1cf6d
Compare
There was a problem hiding this comment.
Great work @annrpom
Reviewed 4 of 65 files at r3, 12 of 59 files at r5, 3 of 3 files at r6.
Reviewable status:
|
tftr! bors r+ |
134826: schemachanger: add support for discarding subzones r=annrpom a=annrpom ### schemachanger: fix discards for explicit txns Previously, discarding a zone config on an object in an explicit transaction would only discard the latest (categorized by highest seq num) element associated with it. This would mean for the following scenario: ``` BEGIN; ALTER TABLE t CONFIGURE ZONE USING num_replicas = 11; ALTER TABLE t CONFIGURE ZONE USING num_replicas = 12; ALTER TABLE t CONFIGURE ZONE DISCARD; COMMIT; ``` The discard would only discard the element with a replication factor of 12, leaving us with a table with a rep. factor of 11. This patch ensures we clean up all references of an element when discarding. Epic: none Release note: None --- ### schemachanger: add support for discarding subzones This patch adds support for discarding a subzone config in the DSC. Fixes: #133158 Epic: none Release note: None Co-authored-by: Annie Pompa <annie@cockroachlabs.com>
|
Build failed: |
|
bors retry (lint tests didn't get to run -- perhaps infra flake?) |
|
Build succeeded: |
schemachanger: fix discards for explicit txns
Previously, discarding a zone config on an object in an
explicit transaction would only discard the latest (categorized by
highest seq num) element associated with it. This would mean for the
following scenario:
The discard would only discard the element with a replication factor of 12,
leaving us with a table with a rep. factor of 11.
This patch ensures we clean up all references of an element when
discarding.
Epic: none
Release note: None
schemachanger: add support for discarding subzones
This patch adds support for discarding a subzone config in the
DSC.
Fixes: #133158
Epic: none
Release note: None