sql/schemachanger: prevent concurrent type desc changes #113639
Conversation
|
This change is |
| ctx, cancel := context.WithCancel(context.Background()) | ||
| dropTypeChan := make(chan struct{}) | ||
| addTypeValueChan := make(chan struct{}) | ||
| var closeAlterPKChanOnce sync.Once |
There was a problem hiding this comment.
Why does this name mean? There doesn't seem to be any Alter PK going on?
| if (len(stmts) == 1 && strings.Contains(stmts[0], "DROP TYPE")) || | ||
| stmts == nil { | ||
| closeAlterPKChanOnce.Do(func() { | ||
| close(addTypeValueChan) |
There was a problem hiding this comment.
nit: These channel names feel backwards? I would expect this one to be the dropType channel. Alternatively you could give them past tense names like typevalueAddedChan?
| @@ -502,6 +562,10 @@ func TestSchemaChangeWaitsForConcurrentSchemaChanges(t *testing.T) { | |||
| tf(t, sessiondatapb.UseNewSchemaChangerOff, sessiondatapb.UseNewSchemaChangerUnsafeAlways) | |||
| }) | |||
|
|
|||
| t.Run("legacy-then-declarative", func(t *testing.T) { | |||
| typeSchemaChangeFunc(t, sessiondatapb.UseNewSchemaChangerOff, sessiondatapb.UseNewSchemaChangerUnsafeAlways) | |||
There was a problem hiding this comment.
Any reason to not run this in the other direction as well?
There was a problem hiding this comment.
nit: Give this a unique t.Run name
| // it will eventually be able to proceed after waiting for a while. | ||
| tdb.Exec(t, `SET use_declarative_schema_changer = $1`, modeFor2ndStmt.String()) | ||
| tdb.Exec(t, `DROP TYPE STATUS;`) | ||
| require.True(t, schemaChangeWaitDetected, "no schema change wait was observed.") |
There was a problem hiding this comment.
This feels a bit race-y to me. schemaChangeWaitDetected might not immediately flipped, right? It might be more straightforward to close another channel and block on that with a timeout?
|
Side note: Is there a reason that type descriptors don't have a |
Previously, the declarative schema changer only waited for legacy schema changer mutations/jobs on relations and did not correctly account for type schema changes. As a result, it was possible to drop a type concurrently while a schema change was trying to mutate a type, which could lead to type change jobs hanging. To address this, this patch causes declarative schema changes to wait for type modifications. Fixes: cockroachdb#112390, cockroachdb#111540 Release note (bug fix): ALTER TYPE could get stuck if DROP TYPE was executed concurrently.
fqazi
force-pushed
the
concurrenTypeChange
branch
from
e709842 to
b443797
Compare
There was a problem hiding this comment.
Side note: Is there a reason that type descriptors don't have a
Mutationsfield? 🤔
Honestly, they probably should have since it would simplify code and be more consistent. Assuming things are using accessors we probably can improve things with declarative work.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @chrisseto)
pkg/sql/schemachanger/schemachanger_test.go line 497 at r1 (raw file):
Previously, chrisseto (Chris Seto) wrote…
Why does this name mean? There doesn't seem to be any Alter PK going on?
Done.
pkg/sql/schemachanger/schemachanger_test.go line 510 at r1 (raw file):
Previously, chrisseto (Chris Seto) wrote…
nit: These channel names feel backwards? I would expect this one to be the
dropTypechannel. Alternatively you could give them past tense names liketypevalueAddedChan?
Good point think these are clearer:
addTypeStartedChan := make(chan struct{})
resumeAddTypeJobChan := make(chan struct{})
pkg/sql/schemachanger/schemachanger_test.go line 550 at r1 (raw file):
Previously, chrisseto (Chris Seto) wrote…
This feels a bit race-y to me.
schemaChangeWaitDetectedmight not immediately flipped, right? It might be more straightforward to close another channel and block on that with a timeout?
Yeah, I'll be more paranoid and close another channel.
pkg/sql/schemachanger/schemachanger_test.go line 566 at r1 (raw file):
Any reason to not run this in the other direction as well?
We can't until we implement these alters in the declarative world.
nit: Give this a unique t.Run name
Renamed this to: typedesc legacy-then-declarative
|
@chrisseto TFTR! bors r+ |
|
Build succeeded: |
There was a problem hiding this comment.
It looks like we cannot amend the existing tf function to include testing for type descriptor because we don't have create type, alter type in declarative schema changer yet. Is this the reason you created a new helper function typeSchemaChangeFunc?
If so, then I'd suggest we move this new test into a new, top-level test (instead of a subtest here), maybe called TestTypeSchemaChangesWaitForConcurrentTypeSchemaChanges, which is cleaner IMO.
Previously, the declarative schema changer only waited for legacy schema changer mutations/jobs on relations and did not correctly account for type schema changes. As a result, it was possible to drop a type concurrently while a schema change was trying to mutate a type, which could lead to type change jobs hanging. To address this, this patch causes declarative schema changes to wait for type modifications.
Fixes: #112390, #111540
Release note (bug fix): ALTER TYPE could get stuck if DROP TYPE
was executed concurrently.