Skip to content
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.

Sign up for GitHub

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

Materializer: normalize schema via schemadiff on --atomic-copy #14636

Merged
merged 2 commits into from
Nov 29, 2023
Merged

Materializer: normalize schema via schemadiff on --atomic-copy #14636

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
36a981a
Materializer: normalize schema via schemadiff on --atomic-copy
shlomi-noach Nov 29, 2023
4a58be6
reduce logging
shlomi-noach Nov 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion go/test/endtoend/vreplication/fk_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ func TestFKWorkflow(t *testing.T) {
}
targetKeyspace := "fktarget"
targetTabletId := 200
vc.AddKeyspace(t, []*Cell{cell}, targetKeyspace, shardName, initialFKTargetVSchema, initialFKSchema, 0, 0, targetTabletId, sourceKsOpts)
vc.AddKeyspace(t, []*Cell{cell}, targetKeyspace, shardName, initialFKTargetVSchema, "", 0, 0, targetTabletId, sourceKsOpts)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change to the test ensures the new behavior is applied. The schema in the test is composed of parent and child tables. Since child < parent lexicographically, the test fails without the new normalization.

vtgate.WaitForStatusOfTabletInShard(fmt.Sprintf("%s.%s.primary", targetKeyspace, shardName), 1, 30*time.Second)

workflowName := "fk"
Expand Down
17 changes: 17 additions & 0 deletions go/vt/vtctl/workflow/materializer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import (
"vitess.io/vitess/go/vt/key"
"vitess.io/vitess/go/vt/log"
"vitess.io/vitess/go/vt/mysqlctl/tmutils"
"vitess.io/vitess/go/vt/schemadiff"
"vitess.io/vitess/go/vt/sqlparser"
"vitess.io/vitess/go/vt/topo"
"vitess.io/vitess/go/vt/vtctl/schematools"
Expand Down Expand Up @@ -444,7 +445,23 @@ func (mz *materializer) deploySchema() error {
}

if len(applyDDLs) > 0 {
if mz.ms.AtomicCopy {
// AtomicCopy suggests we may be interested in Foreign Key support. As such, we want to
// normalize the source schema: ensure the order of table definitions is compatible with
// the constraints graph. We want to first create the parents, then the children.
// We use schemadiff to normalize the schema.
// For now, and because this is could have wider implications, we ignore any errors in
// reading the source schema.
schema, err := schemadiff.NewSchemaFromQueries(applyDDLs)
if err != nil {
log.Error(vterrors.Wrapf(err, "AtomicCopy: failed to normalize schema via schemadiff"))
} else {
applyDDLs = schema.ToQueries()
log.Infof("AtomicCopy used, and schema was normalized via schemadiff. New queries: %v", applyDDLs)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really want to always log this? As it can be a giant schema and thus a huge log entry?

}
}
sql := strings.Join(applyDDLs, ";\n")
log.Infof("materializer.deploySchema: sql=%v", sql)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, this can be very big and also would duplicate the logging.


_, err = mz.tmc.ApplySchema(mz.ctx, targetTablet.Tablet, &tmutils.SchemaChange{
SQL: sql,
Expand Down
17 changes: 17 additions & 0 deletions go/vt/wrangler/materializer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import (
"vitess.io/vitess/go/vt/log"
"vitess.io/vitess/go/vt/mysqlctl/tmutils"
"vitess.io/vitess/go/vt/schema"
"vitess.io/vitess/go/vt/schemadiff"
"vitess.io/vitess/go/vt/sqlparser"
"vitess.io/vitess/go/vt/topo"
"vitess.io/vitess/go/vt/topo/topoproto"
Expand Down Expand Up @@ -1260,7 +1261,23 @@ func (mz *materializer) deploySchema(ctx context.Context) error {
}

if len(applyDDLs) > 0 {
if mz.ms.AtomicCopy {
// AtomicCopy suggests we may be interested in Foreign Key support. As such, we want to
// normalize the source schema: ensure the order of table definitions is compatible with
// the constraints graph. We want to first create the parents, then the children.
// We use schemadiff to normalize the schema.
// For now, and because this is could have wider implications, we ignore any errors in
// reading the source schema.
schema, err := schemadiff.NewSchemaFromQueries(applyDDLs)
if err != nil {
log.Error(vterrors.Wrapf(err, "AtomicCopy: failed to normalize schema via schemadiff"))
} else {
applyDDLs = schema.ToQueries()
log.Infof("AtomicCopy used, and schema was normalized via schemadiff. New queries: %v", applyDDLs)
}
}
sql := strings.Join(applyDDLs, ";\n")
log.Infof("materializer.deploySchema: sql=%v", sql)

_, err = mz.wr.tmc.ApplySchema(ctx, targetTablet.Tablet, &tmutils.SchemaChange{
SQL: sql,
Expand Down
Loading