MoveTables Atomic Mode: set FK checks off while deploying schema on targets #15448
Conversation
Signed-off-by: Rohit Nayak <[email protected]>
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
|
vitess-bot
bot
added
NeedsBackportReason
rohit-nayak-ps
removed
NeedsDescriptionUpdate
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #15448 +/- ##
==========================================
- Coverage 65.67% 65.64% -0.04%
==========================================
Files 1563 1563
Lines 194380 194395 +15
==========================================
- Hits 127658 127601 -57
- Misses 66722 66794 +72 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Rohit Nayak <[email protected]>
Signed-off-by: Rohit Nayak <[email protected]>
rohit-nayak-ps
requested review from
deepthi,
harshit-gangal,
mattlord,
ajm188,
GuptaManan100 and
shlomi-noach
as code owners
rohit-nayak-ps
changed the title
| @@ -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) | |||
There was a problem hiding this comment.
Just noting that this will not work in 5.7, which I think is fine.
| @@ -536,6 +536,10 @@ func (mysqld *Mysqld) ApplySchemaChange(ctx context.Context, dbName string, chan | |||
| sql = "SET sql_log_bin = 0;\n" + sql | |||
| } | |||
|
|
|||
| if change.SetForeignKeyChecksOff { | |||
| sql = "SET foreign_key_checks = 0;\n" + sql | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 GLOBALSo I think it's fine to leave as it is.
There was a problem hiding this comment.
Yes, it’s not about the actual effect in 8.0 but rather being explicit about what your intention is.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I have the same opinion/reasoning as Shlomi
| @@ -536,6 +536,10 @@ func (mysqld *Mysqld) ApplySchemaChange(ctx context.Context, dbName string, chan | |||
| sql = "SET sql_log_bin = 0;\n" + sql | |||
| } | |||
|
|
|||
| if change.SetForeignKeyChecksOff { | |||
| sql = "SET foreign_key_checks = 0;\n" + sql | |||
There was a problem hiding this comment.
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 GLOBALSo I think it's fine to leave as it is.
Signed-off-by: Rohit Nayak <[email protected]>
rohit-nayak-ps
deleted the
rohit/movetables-atomic-mode-fk-constraints-off
branch
Description
Previously schemadiff was conservative in detecting table cycles. It did not allow any two tables which had foreign key constraints pointing to each other, even though there were valid scenarios for this in MySQL. This was fixed in #15431.
However while deploying schemas on the target in MoveTables with atomic mode set, for such scenarios we need to
set foreign_key_checks=0since one of the tables being created will point to one being created in the future. This PR adds the ability to turn off fk constraint checks while deploying schemas, both forvtctlclientandvtctldclientand uses it duringMoveTables Create --atomic-mode.It extends one of the e2e tests which tests imports which use MoveTables for schemas with foreign keys to test this scenario.
Related Issue(s)
Follow-up to #15431 which fixes #15430.
Checklist