-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
schemadiff: using MySQL capabilities to analyze a SchemaDiff and whether changes are applicable instantly/immediately. #14878
schemadiff: using MySQL capabilities to analyze a SchemaDiff and whether changes are applicable instantly/immediately. #14878
Conversation
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
…ff is capable of INSTANT algorithm Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
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.
Looks good! I had one functionality related question with the rest being minor comments/suggestions/questions.
} | ||
} | ||
if strings.EqualFold(col.Type.Type, "set") { | ||
if (len(col.Type.EnumValues)+7)/8 != (len(opt.NewColDefinition.Type.EnumValues)+7)/8 { |
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.
We don't need to perform any math if we're checking for equality here, do we? i.e., I don't think we need to do more than:
if len(col.Type.EnumValues) != len(opt.NewColDefinition.Type.EnumValues) {
Maybe that's not what we want to do though either? I'm assuming it's fine if the new value is less than the existing one. I would think we only care if the new value would be larger / require more space:
if (len(col.Type.EnumValues)+7)/8 < (len(opt.NewColDefinition.Type.EnumValues)+7)/8 {
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.
Unfortunately both assumptions are incorrect. The condition for allowing ALGORITHM=INSTANT
is that the storage space for the SET/ENUM
value is identical.
Here's how storage for 6
values is the same as the storage for 7
values, and INSTANT
is allowed:
> create table t(id int, c1 set('a', 'b', 'c', 'd', 'e', 'f'), primary key(id));
Query OK, 0 rows affected (0.01 sec)
> alter table t modify column c1 set('a', 'b', 'c', 'd', 'e', 'f', 'g'), algorithm=instant;
Query OK, 0 rows affected (0.01 sec)
Records: 0 Duplicates: 0 Warnings: 0
Here's how the storage for 9
values (2 bytes) reduced to 8
values (1 byte) disables INSTANT
:
> create table t(id int, c1 set('a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i'), primary key(id));
Query OK, 0 rows affected (0.01 sec)
> alter table t modify column c1 set('a', 'b', 'c', 'd', 'e', 'f', 'g', 'h'), algorithm=instant;
ERROR 1846 (0A000): ALGORITHM=INSTANT is not supported. Reason: Need to rebuild the table to change column type. Try ALGORITHM=COPY/INPLACE.
for _, diff := range d.UnorderedDiffs() { | ||
capable, err := diffCapableOfInstantDDL(diff, capableOf) | ||
if err != nil { | ||
errs = errors.Join(errs, err) |
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.
Just noting that this uses wrapping (which generally means the outer error was caused by the inner). I'm not sure if that's what we want here?
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.
To the best of my knowledge, errors.Join
is not doing wrapping. It implements a slices of err
objects, most of which are inaccessible unless you explicitly cast your err
to an Unwrap() []error
interface, like so:
Lines 20 to 33 in bd9e534
type Wrapped interface { | |
Unwrap() []error | |
} | |
// Unwrap unwraps an error created by errors.Join() in Go 1.20, into its components | |
func Unwrap(err error) []error { | |
if err == nil { | |
return nil | |
} | |
if u, ok := err.(Wrapped); ok { | |
return u.Unwrap() | |
} | |
return nil | |
} |
Concatenating errors is a paradigm we use throughout schemadiff
, with the explicit intention of returning as much information about diff/problems in one go.
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.
BTW I realize the above code reads Wrapped
and Unwrap
, but I think it's not in the same way you meant it (meaning it's not hierarchical parent->child, but flat). Possibly we should rename those function and type.
Note that if this fixes #14877 -- meaning that it should be closed when this is merged -- then we need to add the Fixes keyword in the Related Issue(s) line. I would have added it myself but not 100% sure that's what you wanted. |
It does not fully fix it. I'll have a followup PR for that! |
Followup PR in #14883 |
Description
See feature description in #14877.
schemadiff
, together withCapableOf
&FlavorCapability
, is able to answer:ALTER TABLE
on a given table schema is eligible forALGORITHM=INSTANT
SchemaDiff
(the diff of two schemas) result is applicable immediately/instantly, meaning all diffs are either trivially immediate (CREATE TABLE
,ALTER VIEW
, etc.) or are eligible forALGORITHM=INSTANT
.This is a first, and isolated, step in a longer change, where:
AlterTableAlgorithmStrategy
hint, we could auto-applyALGORITHM=INSTANT
to alter table statements.onlineddl/analysis.go
, which has overlapping functionality, can then defer toschemadiff
.To reduce
schemadiff
's package dependency, this PR also refactorsCapableOf
&FlavorCapability
out ofgo/vt/mysql
and intogo/vt/mysql/capabilities
.Related Issue(s)
#14877
Checklist
Deployment Notes