OnlineDDL bugfix: make sure schema is applied on tablet #6910
Conversation
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
shlomi-noach
requested review from
harshit-gangal,
sougou and
systay
as code owners
| @@ -605,3 +605,20 @@ func IsConnErr(err error) bool { | |||
| } | |||
| return false | |||
| } | |||
|
|
|||
| // IsSchemaApplyError returns true when given error is a MySQL error applying schema change | |||
| func IsSchemaApplyError(err error) bool { | |||
There was a problem hiding this comment.
Is this error specific to the current use case? I'm wondering why you chose not to use withddl.
There was a problem hiding this comment.
withddl is flawed in that it only applies the schema when there's an error with the query. So, in my case, I ran a SELECT * on the tablet -- that didn't error, but the caller would error because it can't find expected columns.
What If I wanted to add an index? There's no error in the schema, withDDL will not invoke the ADD INDEX statement, but my performance degrades.
And on the flip side, what if I actually dropped a column? If my code no longer uses that column, withDDL will not see any problem and keep that column around.
Regardless, this new IsSchemaApplyError function covers more schema cases than withDDL currently does.
| e.initMutex.Lock() | ||
| defer e.initMutex.Unlock() | ||
|
|
||
| if e.schemaInitialized { |
There was a problem hiding this comment.
We are not allowed to assume that we've "done" something even if we did it. There is one use case where users restore an older snapshot of a mysql instance underneath us. This was the reason why we have to use withddl lazily. We can only know at the time of applying a DML.
There was a problem hiding this comment.
There is one use case where users restore an older snapshot of a mysql instance underneath us.
That's a good point.
Consider looking at how I solved this with orchestrator: it always knows what schema to deploy. See versionIsDeployed function in db.go, which writes down the git sha1 to the backend database.
| @@ -605,3 +605,20 @@ func IsConnErr(err error) bool { | |||
| } | |||
| return false | |||
| } | |||
|
|
|||
| // IsSchemaApplyError returns true when given error is a MySQL error applying schema change | |||
| func IsSchemaApplyError(err error) bool { | |||
| e.initMutex.Lock() | ||
| defer e.initMutex.Unlock() | ||
|
|
||
| if e.schemaInitialized { |
|
I will look into fixing |
co-submission of #6909
This fixes a bug where the
mastertablet may beread_onlywhen attempting to apply the online DDL schema (_vt.schema_migrationstable).With this PR we ensure schema is deployed before any online DDL operation.
It's noteworthy that we move away from
WithDDL, which was meant to solve this exact problem, but is not strict enough. For this PR, we choose a smaller fix, but later on we may want to improve uponWithDDL.