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

lightning: disable foreign key checks #40032

Merged
merged 5 commits into from
Dec 26, 2022
Merged

lightning: disable foreign key checks #40032

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
09c4290
lightning: disable foreign key checks
sleepymole Dec 19, 2022
ea48bcc
Merge branch 'master' into disable-fk-checks
sleepymole Dec 19, 2022
376e42b
do not ignore set statements
sleepymole Dec 21, 2022
188723c
Update br/tests/lightning_foreign_key/config.toml
sleepymole Dec 23, 2022
c4bc87e
Merge branch 'master' into disable-fk-checks
ti-chi-bot Dec 26, 2022
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
52 changes: 10 additions & 42 deletions br/pkg/lightning/restore/tidb.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,10 @@ func DBFromConfig(ctx context.Context, dsn config.DBStore) (*sql.DB, error) {
"tidb_opt_write_row_id": "1",
// always set auto-commit to ON
"autocommit": "1",
// alway set transaction mode to optimistic
// always set transaction mode to optimistic
"tidb_txn_mode": "optimistic",
// disable foreign key checks
"foreign_key_checks": "0",
}

if dsn.Vars != nil {
Expand Down Expand Up @@ -143,47 +145,6 @@ func (timgr *TiDBManager) Close() {
timgr.db.Close()
}

func InitSchema(ctx context.Context, g glue.Glue, database string, tablesSchema map[string]string) error {
logger := log.FromContext(ctx).With(zap.String("db", database))
sqlExecutor := g.GetSQLExecutor()

var createDatabase strings.Builder
createDatabase.WriteString("CREATE DATABASE IF NOT EXISTS ")
common.WriteMySQLIdentifier(&createDatabase, database)
err := sqlExecutor.ExecuteWithLog(ctx, createDatabase.String(), "create database", logger)
if err != nil {
return errors.Trace(err)
}

task := logger.Begin(zap.InfoLevel, "create tables")
var sqlCreateStmts []string
loopCreate:
for tbl, sqlCreateTable := range tablesSchema {
task.Debug("create table", zap.String("schema", sqlCreateTable))

sqlCreateStmts, err = createIfNotExistsStmt(g.GetParser(), sqlCreateTable, database, tbl)
if err != nil {
break
}

// TODO: maybe we should put these createStems into a transaction
for _, s := range sqlCreateStmts {
err = sqlExecutor.ExecuteWithLog(
ctx,
s,
"create table",
logger.With(zap.String("table", common.UniqueTable(database, tbl))),
)
if err != nil {
break loopCreate
}
}
}
task.End(zap.ErrorLevel, err)

return errors.Trace(err)
}

func createIfNotExistsStmt(p *parser.Parser, createTable, dbName, tblName string) ([]string, error) {
stmts, _, err := p.ParseSQL(createTable)
if err != nil {
Expand All @@ -194,11 +155,15 @@ func createIfNotExistsStmt(p *parser.Parser, createTable, dbName, tblName string
ctx := format.NewRestoreCtx(format.DefaultRestoreFlags|format.RestoreTiDBSpecialComment, &res)

retStmts := make([]string, 0, len(stmts))
loop:
for _, stmt := range stmts {
switch node := stmt.(type) {
case *ast.CreateDatabaseStmt:
node.Name = model.NewCIStr(dbName)
node.IfNotExists = true
case *ast.DropDatabaseStmt:
node.Name = model.NewCIStr(dbName)
node.IfExists = true
case *ast.CreateTableStmt:
node.Table.Schema = model.NewCIStr(dbName)
node.Table.Name = model.NewCIStr(tblName)
Expand All @@ -210,6 +175,9 @@ func createIfNotExistsStmt(p *parser.Parser, createTable, dbName, tblName string
node.Tables[0].Schema = model.NewCIStr(dbName)
node.Tables[0].Name = model.NewCIStr(tblName)
node.IfExists = true
default:
// ignore other statements
continue loop
}
if err := stmt.Restore(ctx); err != nil {
return []string{}, common.ErrInvalidSchemaStmt.Wrap(err).GenWithStackByArgs(createTable)
Expand Down
106 changes: 2 additions & 104 deletions br/pkg/lightning/restore/tidb_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,10 +120,8 @@ func TestCreateTableIfNotExistsStmt(t *testing.T) {
require.Equal(t, []string{"CREATE TABLE IF NOT EXISTS `testdb`.`ba``r` (`x` INT);"},
createSQLIfNotExistsStmt("create table foo(x int);", "ba`r"))

// conditional comments
// conditional comments should be removed
require.Equal(t, []string{
"SET NAMES 'binary';",
"SET @@SESSION.`FOREIGN_KEY_CHECKS`=0;",
"CREATE TABLE IF NOT EXISTS `testdb`.`m` (`z` DOUBLE) ENGINE = InnoDB AUTO_INCREMENT = 8343230 DEFAULT CHARACTER SET = UTF8;",
},
createSQLIfNotExistsStmt(`
Expand All @@ -133,20 +131,11 @@ func TestCreateTableIfNotExistsStmt(t *testing.T) {
`, "m"))

// create view
// all set statements are ignored
require.Equal(t, []string{
"SET NAMES 'binary';",
"DROP TABLE IF EXISTS `testdb`.`m`;",
"DROP VIEW IF EXISTS `testdb`.`m`;",
"SET @`PREV_CHARACTER_SET_CLIENT`=@@`character_set_client`;",
"SET @`PREV_CHARACTER_SET_RESULTS`=@@`character_set_results`;",
"SET @`PREV_COLLATION_CONNECTION`=@@`collation_connection`;",
"SET @@SESSION.`character_set_client`=`utf8`;",
"SET @@SESSION.`character_set_results`=`utf8`;",
"SET @@SESSION.`collation_connection`=`utf8_general_ci`;",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this correct for create view sql? I'm not so sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mydump.ExportStatment ignores all executable comments like /*!40014 SET FOREIGN_KEY_CHECKS=0*/.

tidb/br/pkg/lightning/mydump/reader.go

Line 108 in 4a72171

if !(strings.HasPrefix(statement, "/*") && strings.HasSuffix(statement, "*/;")) {

In common cases, these set statements are written as comments. So I guess it's safe to ignore the set statements explicitly.

Copy link
Contributor

Choose a reason for hiding this comment

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

These set statements are not written as comments for mydumper and dumpling. They are real sqls in sql files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. But I think they are actually equivalent to the statements in the comments. It is inconsistent to ignore one while keeping the other.

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe check gbk integration tests to see if character_set_client can be written to gbk, so lightning must execute the statement

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe check gbk integration tests to see if character_set_client can be written to gbk, so lightning must execute the statement

The lightning_new_collation seems to pass.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we allow any SQL statements in db/table schema files, it's hard for us to control the right behavior, such as charset, collate, and foreign checks. Also, it may lead to some security issues.

What about just allow set statement? Because it's a view sql feature. Or I think we can ignore all set statement in sqls but execute these set sqls separately for create view statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that these SET statements will most likely not work with the local backend.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about just allow set statement? Because it's a view sql feature. Or I think we can ignore all set statement in sqls but execute these set sqls separately for create view statement.

@lichunzhu What set statement is needed for view sql?

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 revert the change that ignores set statements. If the user adds some other type of SQL statement to the schema file, it is their responsibility to make sure that these statements do not affect the other logic.

@lichunzhu @lance6716 PTAL

"CREATE ALGORITHM = UNDEFINED DEFINER = `root`@`192.168.198.178` SQL SECURITY DEFINER VIEW `testdb`.`m` (`s`) AS SELECT `s` FROM `db1`.`v1` WHERE `i`<2;",
"SET @@SESSION.`character_set_client`=@`PREV_CHARACTER_SET_CLIENT`;",
"SET @@SESSION.`character_set_results`=@`PREV_CHARACTER_SET_RESULTS`;",
"SET @@SESSION.`collation_connection`=@`PREV_COLLATION_CONNECTION`;",
},
createSQLIfNotExistsStmt(`
/*!40101 SET NAMES binary*/;
Expand All @@ -165,97 +154,6 @@ func TestCreateTableIfNotExistsStmt(t *testing.T) {
`, "m"))
}

func TestInitSchema(t *testing.T) {
s := newTiDBSuite(t)
ctx := context.Background()

s.mockDB.
ExpectExec("CREATE DATABASE IF NOT EXISTS `db`").
WillReturnResult(sqlmock.NewResult(1, 1))
s.mockDB.
ExpectExec("\\QCREATE TABLE IF NOT EXISTS `db`.`t1` (`a` INT PRIMARY KEY,`b` VARCHAR(200));\\E").
WillReturnResult(sqlmock.NewResult(2, 1))
s.mockDB.
ExpectExec("\\QSET @@SESSION.`FOREIGN_KEY_CHECKS`=0;\\E").
WillReturnResult(sqlmock.NewResult(0, 0))
s.mockDB.
ExpectExec("\\QCREATE TABLE IF NOT EXISTS `db`.`t2` (`xx` TEXT) AUTO_INCREMENT = 11203;\\E").
WillReturnResult(sqlmock.NewResult(2, 1))
s.mockDB.
ExpectClose()

s.mockDB.MatchExpectationsInOrder(false) // maps are unordered.
err := InitSchema(ctx, s.tiGlue, "db", map[string]string{
"t1": "create table t1 (a int primary key, b varchar(200));",
"t2": "/*!40014 SET FOREIGN_KEY_CHECKS=0*/;CREATE TABLE `db`.`t2` (xx TEXT) AUTO_INCREMENT=11203;",
})
s.mockDB.MatchExpectationsInOrder(true)
require.NoError(t, err)
}

func TestInitSchemaSyntaxError(t *testing.T) {
s := newTiDBSuite(t)
ctx := context.Background()

s.mockDB.
ExpectExec("CREATE DATABASE IF NOT EXISTS `db`").
WillReturnResult(sqlmock.NewResult(1, 1))
s.mockDB.
ExpectClose()

err := InitSchema(ctx, s.tiGlue, "db", map[string]string{
"t1": "create table `t1` with invalid syntax;",
})
require.Error(t, err)
}

func TestInitSchemaErrorLost(t *testing.T) {
s := newTiDBSuite(t)
ctx := context.Background()

s.mockDB.
ExpectExec("CREATE DATABASE IF NOT EXISTS `db`").
WillReturnResult(sqlmock.NewResult(1, 1))

s.mockDB.
ExpectExec("CREATE TABLE IF NOT EXISTS.*").
WillReturnError(&mysql.MySQLError{
Number: tmysql.ErrTooBigFieldlength,
Message: "Column length too big",
})

s.mockDB.
ExpectClose()

err := InitSchema(ctx, s.tiGlue, "db", map[string]string{
"t1": "create table `t1` (a int);",
"t2": "create table t2 (a int primary key, b varchar(200));",
})
require.Regexp(t, ".*Column length too big.*", err.Error())
}

func TestInitSchemaUnsupportedSchemaError(t *testing.T) {
s := newTiDBSuite(t)
ctx := context.Background()

s.mockDB.
ExpectExec("CREATE DATABASE IF NOT EXISTS `db`").
WillReturnResult(sqlmock.NewResult(1, 1))
s.mockDB.
ExpectExec("CREATE TABLE IF NOT EXISTS `db`.`t1`.*").
WillReturnError(&mysql.MySQLError{
Number: tmysql.ErrTooBigFieldlength,
Message: "Column length too big",
})
s.mockDB.
ExpectClose()

err := InitSchema(ctx, s.tiGlue, "db", map[string]string{
"t1": "create table `t1` (a VARCHAR(999999999));",
})
require.Regexp(t, ".*Column length too big.*", err.Error())
}

func TestDropTable(t *testing.T) {
s := newTiDBSuite(t)
ctx := context.Background()
Expand Down
4 changes: 4 additions & 0 deletions br/tests/lightning_foreign_key/config.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
[tikv-importer]
# Set on-duplicate=error to force using insert statement to write data.
# It seems that foreign key check is not supported in replace statement.
Copy link
Contributor

Choose a reason for hiding this comment

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

didn't find the document about foreign_key_check with REPLACE INTO, could you explain what's the behaviour?

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 haven't delved into it yet. I just tested it with the latest TiDB and it doesn't seem to work with REPLACE INTO yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

cc @crazycs520

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch, it is a bug of TiDB, foreign_key_check should work with REPLACE INTO too, I will fix this as soon as possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

fix in #40069

on-duplicate = "error"
8 changes: 8 additions & 0 deletions br/tests/lightning_foreign_key/data/fk.t-schema.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
CREATE TABLE `t`
(
`a` bigint(20) NOT NULL,
`b` bigint(20) DEFAULT NULL,
PRIMARY KEY (`a`) /*T![clustered_index] CLUSTERED */,
KEY `fk_1` (`b`),
CONSTRAINT `fk_1` FOREIGN KEY (`b`) REFERENCES `test`.`t2` (`a`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin;
6 changes: 6 additions & 0 deletions br/tests/lightning_foreign_key/data/fk.t.csv
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
a,b
1,1
2,2
3,3
4,4
5,5
28 changes: 28 additions & 0 deletions br/tests/lightning_foreign_key/run.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
#!/bin/bash
#
# Copyright 2022 PingCAP, Inc.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

set -eu

# Create existing tables that import data will reference.
run_sql 'CREATE DATABASE IF NOT EXISTS fk;'
run_sql 'CREATE TABLE fk.t2 (a BIGINT PRIMARY KEY);'

for BACKEND in tidb local; do
run_sql 'DROP TABLE IF EXISTS fk.t;'
run_lightning --backend $BACKEND
run_sql 'SELECT GROUP_CONCAT(a) FROM fk.t ORDER BY a;'
check_contains '1,2,3,4,5'
done