sql: fix bad interraction with DROP region rollback and alter to RBR #63525
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1, 2 of 2 files at r2.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @ajstorm and @arulajmani)
pkg/ccl/multiregionccl/region_test.go, line 221 at r2 (raw file):
defer sqltestutils.SetTestJobsAdoptInterval()() testCases := []struct {
should we test the other way as well, e.g. REGIONAL/ GLOBAL blocks ADD/DROP REGION?
can we also test REGIONAL BY ROW -> GLOBAL rollbacks behave sanely?
what about CREATE INDEX/ALTER PRIMARY KEY/ADD COLUMN ... UNIQUE/ADD UNIQUE CONSTRAINT on RBR and the like?
pkg/ccl/multiregionccl/region_test.go, line 227 at r2 (raw file):
}{ { "region-drop-alter-from-global",
nit: can you put the keys names in here? e.g. name:, setup:, ...
it's easier to read. (and if goland does something fancy, it doesn't show up in code reviews or non-rich people who can't pay for Goland!)
pkg/ccl/multiregionccl/region_test.go, line 268 at r2 (raw file):
} _, sqlDB, cleanup := multiregionccltestutils.TestingCreateMultiRegionCluster(
if you want to speed this test up, i'd put it outside the loop and defer a DROP DATABASE t cleanup on each run of t.Run(tc.name, func(t *testing.T) {.
pkg/sql/multiregion_finalizer.go, line 11 at r1 (raw file):
// licenses/APL.txt. package sql
nit: multi_region_finalizer.go
pkg/sql/multiregion_finalizer.go, line 34 at r2 (raw file):
} // NewMultiRegionFinalizer returns a multiRegionFinalizer.
can you describe the purpose of this?
pkg/sql/multiregion_finalizer.go, line 70 at r2 (raw file):
return ApplyZoneConfigFromDatabaseRegionConfig( ctx, typeDesc.GetParentID(),
we're fetching the type just to get the database id. can the finalizer take in the database id to avoid fetching the type descriptor just for this?
pkg/sql/multiregion_finalizer.go, line 86 at r2 (raw file):
var repartitionedTableIDs []descpb.ID typeDesc, err := descsCol.GetImmutableTypeByID(ctx, txn, r.typeID, tree.ObjectLookupFlags{})
we're fetching the type just to get the database id. can the finalizer take in the database id to avoid fetching the type descriptor just for this?
|
pkg/sql/multiregion_finalizer.go, line 34 at r2 (raw file): Previously, otan (Oliver Tan) wrote…
where this = put a comment on what the multiRegionFinalizer does. this should also be private, i.e. |
|
pkg/sql/multiregion_finalizer.go, line 34 at r2 (raw file): Previously, otan (Oliver Tan) wrote…
databaseRegionChangeFinaliser maybe? Multi region finalizer is a but too vague for me |
There was a problem hiding this comment.
Reviewable status:
pkg/ccl/multiregionccl/region_test.go, line 221 at r2 (raw file):
should we test the other way as well, e.g. REGIONAL/ GLOBAL blocks ADD/DROP REGION?
can we also test REGIONAL BY ROW -> GLOBAL rollbacks behave sanely?
Yup, saving this for a separate PR though.
what about CREATE INDEX/ALTER PRIMARY KEY/ADD COLUMN ... UNIQUE/ADD UNIQUE CONSTRAINT on RBR and the like?
I'll add a third commit here for this -- I'm planning on doing this in a new test instead of adding more configurable knobs here for readability.
pkg/ccl/multiregionccl/region_test.go, line 227 at r2 (raw file):
Previously, otan (Oliver Tan) wrote…
nit: can you put the keys names in here? e.g. name:, setup:, ...
it's easier to read. (and if goland does something fancy, it doesn't show up in code reviews or non-rich people who can't pay for Goland!)
Done! It does indeed haha
pkg/sql/multiregion_finalizer.go, line 34 at r2 (raw file):
Previously, otan (Oliver Tan) wrote…
databaseRegionChangeFinaliser maybe? Multi region finalizer is a but too vague for me
Done.
pkg/sql/multiregion_finalizer.go, line 70 at r2 (raw file):
Previously, otan (Oliver Tan) wrote…
we're fetching the type just to get the database id. can the finalizer take in the database id to avoid fetching the type descriptor just for this?
Done.
pkg/sql/multiregion_finalizer.go, line 86 at r2 (raw file):
Previously, otan (Oliver Tan) wrote…
we're fetching the type just to get the database id. can the finalizer take in the database id to avoid fetching the type descriptor just for this?
There's this assertion in there too to make sure the type desc is of the correct kind.
There was a problem hiding this comment.
Reviewable status:
pkg/ccl/multiregionccl/region_test.go, line 221 at r2 (raw file):
I'll add a third commit here for this -- I'm planning on doing this in a new test instead of adding more configurable knobs here for readability.
On second thought, there seems to be quite a few permutations here. How do you feel about splitting these up into issues to keep PRs manageable? I suspect most would be adding tests.
There was a problem hiding this comment.
Nit: capitalization with the second RBR in the release note.
Also, you say that this informs #63462. What's left in that issue once this is merged?
Especially since this is so late in the release cycle, it would also be good to get @ajwerner's 👀 on this one.
Reviewed 1 of 2 files at r2, 1 of 4 files at r3.
Reviewable status:
pkg/ccl/multiregionccl/region_test.go, line 216 at r2 (raw file):
defer log.Scope(t).Close(t) skip.UnderRace(t, "times out under race")
Are you assuming this? Or was this caught as part of CI?
pkg/ccl/multiregionccl/region_test.go, line 221 at r2 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
I'll add a third commit here for this -- I'm planning on doing this in a new test instead of adding more configurable knobs here for readability.
On second thought, there seems to be quite a few permutations here. How do you feel about splitting these up into issues to keep PRs manageable? I suspect most would be adding tests.
I'm good with splitting up to multiple PRs. Might want to open a tracking issue so we don't lose track of some of them.
What about RBR->RBR? Is that worth testing? As well as create RBR?
pkg/sql/database_region_change_finalizer.go, line 82 at r3 (raw file):
// repartitionRegionalByRowTables re-partitions all REGIONAL BY ROW tables // enclosed inside the database of its multi-region enum. There is a partition
Nit: "inside the database of its multi-region enum" -> "contained in the database"? I'm not following the "of its multi-region enum" part. Also, the second sentence might read better if it started off with "This function guarantees that...".
There was a problem hiding this comment.
Reviewed 2 of 4 files at r3.
Reviewable status:
pkg/ccl/multiregionccl/region_test.go, line 216 at r2 (raw file):
Previously, ajstorm (Adam Storm) wrote…
Are you assuming this? Or was this caught as part of CI?
give it a go without...
you'll need the speedup i mentioned below
pkg/sql/type_change.go, line 368 at r3 (raw file):
} } multiRegionFinalizer = newDatabaseRegionChangeFinalizer(typeDesc.GetParentID(), typeDesc.GetID())
nit: databaseRegionChangeFinalizer
pkg/sql/database_region_change_finalizer.go, line 244 at r3 (raw file):
tbID, r.typeID, )
missing a continue?
pkg/sql/multiregion_finalizer.go, line 86 at r2 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
There's this assertion in there too to make sure the type desc is of the correct kind.
i still think it's unnecessary. if we're pedantic, make the init take in a typeDesc and ensure it is multi region.
synthesise region config errors anyway if it's not multi-region iirc.
There was a problem hiding this comment.
What about RBR->RBR? Is that worth testing? As well as create RBR?
Right now, in this test setup, creating a RBR or transitioning from a RBR -> RBR doesn't work -- it blocks. I don't know why GLOBAL->RBR works but RBR->RBR doesn't -- I'll dig into it more tomorrow.
Reviewable status:
There was a problem hiding this comment.
Reviewable status:
pkg/ccl/multiregionccl/region_test.go, line 216 at r2 (raw file):
Previously, otan (Oliver Tan) wrote…
give it a go without...
you'll need the speedup i mentioned below
I don't think we need this for this PR, but we should at least attempt it. It's more important that we get this in relatively quickly, as it seems to be one of our last known GA blockers.
arulajmani
requested review from
a team and
adityamaru
and removed request for
a team
There was a problem hiding this comment.
Also, you say that this informs #63462. What's left in that issue once this is merged?
A couple of things --
- This PR sandwiches ALTER TO REGIONAL BY ROW in the enclosing ADD/DROP regions. We still need to fix (and test) the other way around -- sandwiching ADD/DROP REGION in an enclosing ALTER TO REGIONAL BY ROW operation.
- Need to spruce up testing for some of the things discussed in the PR. I imagined I'd do it as part of this umbrella issue, but if we want to remove the release blocker label I'm happy to create new issues.
Especially since this is so late in the release cycle, it would also be good to get @ajwerner's 👀 on this one.
I'll add him!
Sorry this took a bit, ended up going down some debugging rabbit holes for this yesterday. :) RFAL
Reviewable status:
pkg/ccl/multiregionccl/region_test.go, line 216 at r2 (raw file):
Previously, ajstorm (Adam Storm) wrote…
I don't think we need this for this PR, but we should at least attempt it. It's more important that we get this in relatively quickly, as it seems to be one of our last known GA blockers.
Pulled out the cluster setup outside the loop, let's see if this works :)
pkg/ccl/multiregionccl/region_test.go, line 221 at r2 (raw file):
Previously, ajstorm (Adam Storm) wrote…
I'm good with splitting up to multiple PRs. Might want to open a tracking issue so we don't lose track of some of them.
What about RBR->RBR? Is that worth testing? As well as create RBR?
Added RBR->RBR and explicit regional. I think CREATE RBR should probably go in with the test that does CREATE INDEX/ALTER PRIMARY KEY/etc.
pkg/ccl/multiregionccl/region_test.go, line 268 at r2 (raw file):
Previously, otan (Oliver Tan) wrote…
if you want to speed this test up, i'd put it outside the loop and defer a
DROP DATABASE tcleanup on each run oft.Run(tc.name, func(t *testing.T) {.
Figured out why this was blocking when we were debugging the other day -- it was because the testing knob we were using earlier is hit int he DROP DATABASE CASCADE path as well. :P
pkg/sql/database_region_change_finalizer.go, line 244 at r3 (raw file):
Previously, otan (Oliver Tan) wrote…
missing a continue?
Good catch!
pkg/sql/multiregion_finalizer.go, line 86 at r2 (raw file):
Previously, otan (Oliver Tan) wrote…
i still think it's unnecessary. if we're pedantic, make the init take in a typeDesc and ensure it is multi region.
synthesise region config errors anyway if it's not multi-region iirc.
Done.
There was a problem hiding this comment.
Reviewed 3 of 5 files at r4.
Reviewable status:
pkg/ccl/multiregionccl/region_test.go, line 268 at r2 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
Figured out why this was blocking when we were debugging the other day -- it was because the testing knob we were using earlier is hit int he
DROP DATABASE CASCADEpath as well. :P
I find having a fresh cluster per subtest is not that much overhead in terms of time and can help shake out flakes.
pkg/ccl/multiregionccl/region_test.go, line 260 at r4 (raw file):
typeChangeStarted := make(chan struct{}) typeChangeFinished := make(chan struct{}) rbrOpFinished := make(chan struct{})
you may want to give these some capacity. Without having read this in depth, my senses are tingling that the synchronization in this test in insufficient. Run just this test under stress for a while and make sure it doesn't hang.
pkg/sql/type_change.go, line 299 at r4 (raw file):
for _, member := range typeDesc.EnumMembers { if t.isTransitioningInCurrentJob(&member) && enumMemberIsRemoving(&member) { if err := t.canRemoveEnumValue(ctx, typeDesc, txn, &member, descsCol); err != nil {
so wait, say this returns an error, meaning you cannot remove this value and you had a concurrent schema change to transition to regional by row that saw this value as dropping so didn't partition for it, won't we just early return, hit OnFailOrCancel and then do nothing?
pkg/sql/type_change.go, line 452 at r4 (raw file):
var regionChangeFinalizer *databaseRegionChangeFinalizer // Cleanup:
nit: why this newline?
pkg/sql/type_change.go, line 458 at r4 (raw file):
return err } b := txn.NewBatch()
I'm really not clear on why we're using the batch API here given we only update a single descriptor.
pkg/sql/database_region_change_finalizer.go, line 244 at r3 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
Good catch!
nit: maybe just invert it?
if err := WaitToUpdateLeases(ctx, leaseMgr, tbID); err != nil {
if !errors.Is(err, catalog.ErrDescriptorNotFound) {
return err
}
// Swallow
log.Infof(...
pkg/sql/multiregion_finalizer.go, line 34 at r2 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
Done.
Why is this even a struct? There are two methods which are ever invoked and this is really just about passing arguments to it.
There was a problem hiding this comment.
Need to spruce up testing for some of the things discussed in the PR. I imagined I'd do it as part of this umbrella issue, but if we want to remove the release blocker label I'm happy to create new issues.
I'm happy to keep this all under the tracking issue.
Reviewable status:
This thing was getting slightly unwieldy, so it makes sense to encapsulate all the finalization logic in one place and a separate file. Inspired by otan's draft PR cockroachdb#63459. Release note: None
There was a problem hiding this comment.
Reviewable status:
pkg/ccl/multiregionccl/region_test.go, line 268 at r2 (raw file):
Previously, ajwerner wrote…
I find having a fresh cluster per subtest is not that much overhead in terms of time and can help shake out flakes.
I've found whenever I have ~3 or more subtests, each with their own cluster, it tends to time out under race
pkg/ccl/multiregionccl/region_test.go, line 260 at r4 (raw file):
Previously, ajwerner wrote…
typeChangeStarted := make(chan struct{}) typeChangeFinished := make(chan struct{}) rbrOpFinished := make(chan struct{})you may want to give these some capacity. Without having read this in depth, my senses are tingling that the synchronization in this test in insufficient. Run just this test under stress for a while and make sure it doesn't hang.
Stressed this thing for ~20 minutes and it seems to be doing fine. I'll add a capacity of 1 nonetheless
pkg/sql/type_change.go, line 299 at r4 (raw file):
Previously, ajwerner wrote…
so wait, say this returns an error, meaning you cannot remove this value and you had a concurrent schema change to transition to regional by row that saw this value as dropping so didn't partition for it, won't we just early return, hit OnFailOrCancel and then do nothing?
That was exactly what was happening before my patch. With my changes, OnFailOrCancel now repartitions all regional by row tables right after promoting the value we hoped to drop back to PUBLIC.
pkg/sql/type_change.go, line 458 at r4 (raw file):
Previously, ajwerner wrote…
I'm really not clear on why we're using the batch API here given we only update a single descriptor.
I don't think this was done with much intention -- fixed
pkg/sql/database_region_change_finalizer.go, line 244 at r3 (raw file):
Previously, ajwerner wrote…
nit: maybe just invert it?
if err := WaitToUpdateLeases(ctx, leaseMgr, tbID); err != nil { if !errors.Is(err, catalog.ErrDescriptorNotFound) { return err } // Swallow log.Infof(...
Done.
pkg/sql/multiregion_finalizer.go, line 34 at r2 (raw file):
Previously, ajwerner wrote…
Why is this even a struct? There are two methods which are ever invoked and this is really just about passing arguments to it.
Mostly as a way to store the set of repartionedTables between the two phases (updating zone configs and waiting for leases to drain). I can revert back to simple functions if you think this feels unnecessary.
There was a problem hiding this comment.
Reviewable status:
pkg/ccl/multiregionccl/region_test.go, line 268 at r2 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
I've found whenever I have ~3 or more subtests, each with their own cluster, it tends to time out under race
That's very surprising. That feels worthy of an issue. Looking at CI, it seems like this thing is timing out.
pkg/sql/type_change.go, line 299 at r4 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
That was exactly what was happening before my patch. With my changes,
OnFailOrCancelnow repartitions all regional by row tables right after promoting the value we hoped to drop back toPUBLIC.
Got it, I missed some of the logic.
pkg/sql/database_region_change_finalizer.go, line 156 at r5 (raw file):
// Update the index descriptor proto's partitioning. index.IndexDesc().Partitioning = newIdx.Partitioning
maybe instead call SetIndex?
pkg/sql/multiregion_finalizer.go, line 34 at r2 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
Mostly as a way to store the set of
repartionedTablesbetween the two phases (updating zone configs and waiting for leases to drain). I can revert back to simple functions if you think this feels unnecessary.
Fine by me.
When altering to a regional by row table we only add partitions for PUBLIC enum values (regions). The type schema change job that negotiates promotion/demotion is responsible for re-partitioning regional by row tables. Previously, this re-partitioning only happened if the type schema change job succeeded. This behavior missed an edge case, which looks like: - Client 1 drops region A from the database. The user txn commits. - Client 2 alters table t to a RBR. - The drop region fails, and therefore rolls back. - table t is now missing a partition for region A. This patch fixes this problem by trigerring a re-partition even when the type schema change job is rolling back. Informs cockroachdb#63462 Release note (bug fix): Previously if a user altered a table to REGIONAL BY ROW when a region was being dropped, and the drop failed and had to be rolled back, it could have resulted in the Regional By Row table missing a partition for this region. This is now fixed and region drop failure rollbacks are sane.
There was a problem hiding this comment.
Reviewable status:
pkg/ccl/multiregionccl/region_test.go, line 268 at r2 (raw file):
Previously, ajwerner wrote…
That's very surprising. That feels worthy of an issue. Looking at CI, it seems like this thing is timing out.
Filed this: #63791
pkg/sql/database_region_change_finalizer.go, line 156 at r5 (raw file):
Previously, ajwerner wrote…
maybe instead call SetIndex?
Discussed offline, keeping this thing as is because there isn't a SetIndex variant for indexes with mutations on them. Now that I look at this again, just re-partitioning non-drop indexes doesn't seem like the right thing to do as the index drop could fail -- I'll fix this when I add tests for these things, which is coming up next.
|
Thanks for the reviews! bors r=ajwerner |
|
Build succeeded: |
See individual commits for details.