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

MoveTables Atomic Mode: set FK checks off while deploying schema on targets #15448

Merged
merged 4 commits into from
Mar 12, 2024
Merged

MoveTables Atomic Mode: set FK checks off while deploying schema on targets #15448

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7c23bae
Set FK checks off while creating schema in MoveTables atomic mode.
rohit-nayak-ps Mar 11, 2024
ad71d89
Self-review
rohit-nayak-ps Mar 11, 2024
eccec5b
Make proto
rohit-nayak-ps Mar 11, 2024
2a9518d
Address review comments
rohit-nayak-ps Mar 12, 2024
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
29 changes: 28 additions & 1 deletion go/test/endtoend/vreplication/fk_config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,19 +16,28 @@ limitations under the License.

package vreplication

// The tables in the schema are selected so that we have one parent/child table with names in reverse lexical order
// (child before parent), t1,t2 are in lexical order, and t11,t12 have valid circular foreign key constraints.
var (
initialFKSchema = `
create table parent(id int, name varchar(128), primary key(id)) engine=innodb;
create table child(id int, parent_id int, name varchar(128), primary key(id), foreign key(parent_id) references parent(id) on delete cascade) engine=innodb;
create view vparent as select * from parent;
create table t1(id int, name varchar(128), primary key(id)) engine=innodb;
create table t2(id int, t1id int, name varchar(128), primary key(id), foreign key(t1id) references t1(id) on delete cascade) engine=innodb;
create table t11 (id int primary key, i int);
create table t12 (id int primary key, i int);
alter table t11 add constraint f11 foreign key (i) references t12 (id);
alter table t12 add constraint f12 foreign key (i) references t11 (id);
`
initialFKData = `
insert into parent values(1, 'parent1'), (2, 'parent2');
insert into child values(1, 1, 'child11'), (2, 1, 'child21'), (3, 2, 'child32');
insert into t1 values(1, 't11'), (2, 't12');
insert into t2 values(1, 1, 't21'), (2, 1, 't22'), (3, 2, 't23');
insert into t11 values(1, null);
insert into t12 values(1, 1);
update t11 set i = 1 where id = 1;
`

initialFKSourceVSchema = `
Expand All @@ -37,7 +46,9 @@ insert into t2 values(1, 1, 't21'), (2, 1, 't22'), (3, 2, 't23');
"parent": {},
"child": {},
"t1": {},
"t2": {}
"t2": {},
"t11": {},
"t12": {}
}
}
`
Expand Down Expand Up @@ -82,6 +93,22 @@ insert into t2 values(1, 1, 't21'), (2, 1, 't22'), (3, 2, 't23');
"name": "reverse_bits"
}
]
},
"t11": {
"column_vindexes": [
{
"column": "id",
"name": "reverse_bits"
}
]
},
"t12": {
"column_vindexes": [
{
"column": "id",
"name": "reverse_bits"
}
]
}
}
}
Expand Down
39 changes: 30 additions & 9 deletions go/test/endtoend/vreplication/fk_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,9 @@ func TestFKWorkflow(t *testing.T) {
require.NotNil(t, targetTab)
catchup(t, targetTab, workflowName, "MoveTables")
vdiff(t, targetKeyspace, workflowName, cellName, true, false, nil)
ls.waitForAdditionalRows(200)
if withLoad {
ls.waitForAdditionalRows(200)
}
vdiff(t, targetKeyspace, workflowName, cellName, true, false, nil)
if withLoad {
cancel()
Expand All @@ -121,13 +123,19 @@ func TestFKWorkflow(t *testing.T) {
ls = newFKLoadSimulator(t, ctx)
defer cancel()
go ls.simulateLoad()
}
ls.waitForAdditionalRows(200)
if withLoad {
ls.waitForAdditionalRows(200)
cancel()
<-ch
}
mt.Complete()
vtgateConn, closeConn := getVTGateConn()
defer closeConn()

t11Count := getRowCount(t, vtgateConn, "t11")
t12Count := getRowCount(t, vtgateConn, "t12")
require.Greater(t, t11Count, 1)
require.Greater(t, t12Count, 1)
require.Equal(t, t11Count, t12Count)
}

func insertInitialFKData(t *testing.T) {
Expand All @@ -140,11 +148,18 @@ func insertInitialFKData(t *testing.T) {
log.Infof("Inserting initial FK data")
execMultipleQueries(t, vtgateConn, db, initialFKData)
log.Infof("Done inserting initial FK data")
waitForRowCount(t, vtgateConn, db, "parent", 2)
waitForRowCount(t, vtgateConn, db, "child", 3)
waitForRowCount(t, vtgateConn, db, "t1", 2)
waitForRowCount(t, vtgateConn, db, "t2", 3)

type tableCounts struct {
name string
count int
}
for _, table := range []tableCounts{
{"parent", 2}, {"child", 3},
{"t1", 2}, {"t2", 3},
{"t11", 1}, {"t12", 1},
} {
waitForRowCount(t, vtgateConn, db, table.name, table.count)
}
})
}

Expand All @@ -170,6 +185,7 @@ func newFKLoadSimulator(t *testing.T, ctx context.Context) *fkLoadSimulator {
}
}

var indexCounter int = 100 // used to insert into t11 and t12
func (ls *fkLoadSimulator) simulateLoad() {
t := ls.t
var err error
Expand All @@ -193,8 +209,13 @@ func (ls *fkLoadSimulator) simulateLoad() {
default: // 20% chance to delete
ls.delete()
}
for _, table := range []string{"t11", "t12"} {
query := fmt.Sprintf("insert /*+ SET_VAR(foreign_key_checks=0) */ into fksource.%s values(%d, %d)", table, indexCounter, indexCounter)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just noting that this will not work in 5.7, which I think is fine.

ls.exec(query)
indexCounter++
}
require.NoError(t, err)
time.Sleep(1 * time.Millisecond)
time.Sleep(10 * time.Millisecond)
}
}

Expand Down
4 changes: 4 additions & 0 deletions go/vt/mysqlctl/schema.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -536,6 +536,10 @@
sql = "SET sql_log_bin = 0;\n" + sql
}

if change.SetForeignKeyChecksOff {
sql = "SET foreign_key_checks = 0;\n" + sql

Check warning on line 540 in go/vt/mysqlctl/schema.go

View check run for this annotation

Codecov / codecov/patch

go/vt/mysqlctl/schema.go#L539-L540 

Added lines #L539 - L540 were not covered by tests
Copy link
Contributor

@mattlord mattlord Mar 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This variable has a SESSION and GLOBAL context and value. IMO we should be explicit here and use session:

SET @@session.foreign_key_checks = 0;\n" + sql

https://dev.mysql.com/doc/refman/8.0/en/server-system-variables.html#sysvar_foreign_key_checks

Copy link
Contributor

Choose a reason for hiding this comment

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

The syntax, as it is, is setting the session value only. In MySQL, if neither session nor global is specified, then it is presumed to be session. For example:

> set read_only=1;
ERROR 1229 (HY000): Variable 'read_only' is a GLOBAL variable and should be set with SET GLOBAL

So I think it's fine to leave as it is.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it’s not about the actual effect in 8.0 but rather being explicit about what your intention is.

Copy link
Contributor

Choose a reason for hiding this comment

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

What my thinking is, that as MySQL veterans this is explicit enough. It's common practice to say set foreign_key_checks=0 and it's clear that you're intent on modifying the session variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have the same opinion/reasoning as Shlomi

}

// add a 'use XXX' in front of the SQL
sql = fmt.Sprintf("USE %s;\n%s", sqlescape.EscapeID(dbName), sql)

Expand Down
13 changes: 7 additions & 6 deletions go/vt/mysqlctl/tmutils/schema.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,12 +300,13 @@ func DiffSchemaToArray(leftName string, left *tabletmanagerdatapb.SchemaDefiniti
// SchemaChange contains all necessary information to apply a schema change.
// It should not be sent over the wire, it's just a set of parameters.
type SchemaChange struct {
SQL string
Force bool
AllowReplication bool
BeforeSchema *tabletmanagerdatapb.SchemaDefinition
AfterSchema *tabletmanagerdatapb.SchemaDefinition
SQLMode string
SQL string
Force bool
AllowReplication bool
BeforeSchema *tabletmanagerdatapb.SchemaDefinition
AfterSchema *tabletmanagerdatapb.SchemaDefinition
SQLMode string
SetForeignKeyChecksOff bool
rohit-nayak-ps marked this conversation as resolved.
Show resolved Hide resolved
}

// Equal compares two SchemaChange objects.
Expand Down
Loading
Loading