ddl: Add admin check before drop table using building flag. by crazycs520 · Pull Request #7343 · pingcap/tidb

crazycs520 · 2018-08-09T16:11:10Z

What have you changed? (mandatory)

Add admin check table before drop table according to the ldflags when building.

Does this PR affect tidb-ansible update? (mandatory)

No

Does this PR need to be added to the release notes? (mandatory)

No

Refer to a related PR or issue link (optional)

Benchmark result if necessary (optional)

Add a few positive/negative examples (optional)

…igint have admin check bug

…in_check_before_drop

crazycs520 · 2018-08-09T16:23:18Z

@winkyao @zimulala @shenli PTAL

shenli · 2018-08-09T16:25:43Z

executor/ddl.go

 			return errors.Trace(err)
 		}

+		if os.Getenv("TIDB_CHECK_BEFORE_DROP") == "1" {


Please add a log here. I suggest using the warn log level.

shenli · 2018-08-09T16:26:53Z

executor/executor.go

 	defer func() { e.done = true }()
 	dbName := model.NewCIStr(e.ctx.GetSessionVars().CurrentDB)
 	for _, t := range e.tables {
+		if dbName.L == "" {


Is this a bug-fix?

Ye~
If use e.ctx.(sqlexec.RestrictedSQLExecutor).ExecRestrictedSQL(e.ctx, "admin check table test.t1") ,here dbName.L is "".

Could we use t.DBInfo.Name directly?

@crazycs520

shenli · 2018-08-09T16:27:12Z

executor/executor_test.go

 	// For "checksum_without_index", we only have one checksum, so the result will be 1.
 	res.Sort().Check(testkit.Rows("test checksum_with_index 0 2 2", "test checksum_without_index 1 1 1"))

+	tk.MustExec("drop table if exists admin_test")


Could you add a test case that meet error?

winkyao

reset LGTM

shenli · 2018-08-10T02:33:40Z

executor/executor_test.go


+	os.Setenv("TIDB_CHECK_BEFORE_DROP", "1")
+	r, err = tk.Exec("drop table admin_test")
+	c.Assert(err, NotNil)


Please check the content of the error to make sure it is thrown by the consistent checking.

winkyao · 2018-08-10T02:33:47Z

@crazycs520 Please add the proper label.

jackysp · 2018-08-10T02:47:46Z

I think it's better to use a building flag for this check.

zz-jason · 2018-08-10T03:00:51Z

depends on os environment variable is not a good practice.

crazycs520 · 2018-08-10T06:17:32Z

@jackysp @zz-jason @tiancaiamao PTAL

winkyao · 2018-08-10T06:55:00Z

@crazycs520 Please change the pr title.

winkyao · 2018-08-10T07:00:31Z

config/config.go


+func init() {
+	if CheckBeforeDropLDFlag == "1" {
+		CheckBeforeDrop = true


use atomic to protect CheckBeforeDrop, or the data race test perhaps reports error.

winkyao · 2018-08-10T07:00:40Z

executor/executor_test.go

 func (s *testSuite) TestAdmin(c *C) {
+	origin := config.CheckBeforeDrop
+	defer func() {
+		config.CheckBeforeDrop = origin


alivxxx · 2018-08-10T07:56:03Z

util/printer/printer.go

 	log.Infof("UTC Build Time:  %s", TiDBBuildTS)
 	log.Infof("GoVersion:  %s", GoVersion)
 	log.Infof("Race Enabled: %v", israce.RaceEnabled)
+	log.Infof("Admin check table before drop is %v", config.CheckBeforeDrop)


How about Check Table Before Drop: %v?

jackysp · 2018-08-10T08:51:11Z

executor/ddl.go

 		}

+		if config.CheckBeforeDrop {
+			log.Warnf("admin check table before drop was on")


It's better to add more info in this log, e.g. the table name.

jackysp · 2018-08-10T09:01:15Z

executor/executor.go

 	defer func() { e.done = true }()
-	dbName := model.NewCIStr(e.ctx.GetSessionVars().CurrentDB)
 	for _, t := range e.tables {
+		dbName := t.DBInfo.Name


Finally, it was fixed. 👍

Btw, I think we need a test case for it.

winkyao

LGTM

crazycs520 · 2018-08-11T06:51:17Z

@winkyao PTAL again. I add ldflags to test and remove atomic set.

shenli · 2018-08-11T13:24:16Z

config/config.go

 		"tikv":     true,
 	}
+	// checkTableBeforeDrop enable to execute `admin check table` before `drop table`.
+	CheckTableBeforeDrop = true


It should be false by default.

jackysp · 2018-08-11T16:35:38Z

executor/admin_test.go

 	tk.MustExec(`admin check table t;`)
+
+	tk = testkit.NewTestKit(c, s.store)
+	tk.MustExec(`admin check table test.t;`)


It's better to check the table from the other database.

jackysp · 2018-08-13T02:57:34Z

/run-all-tests

jackysp

LGTM

tiancaiamao · 2018-08-13T02:58:24Z

config/config.go

Could this variable be in lower case ?

…in_check_before_drop

zz-jason · 2018-08-13T06:45:25Z

should we also add some UTs to make sure the ldflags take effect?

tiancaiamao · 2018-08-13T06:54:41Z

LGTM
It's hard to use unit test to check ldflags, we have to do it manually or in the integrated test. @zz-jason

zz-jason · 2018-08-13T06:57:58Z

@tiancaiamao If we run make test, the global config config.CheckTableBeforeDrop should always be true, maybe we can check this? @crazycs520 How do you think?

tiancaiamao · 2018-08-14T12:41:32Z

/run-all-tests

zz-jason · 2018-08-14T12:41:36Z

LGTM

crazycs520 added 4 commits August 8, 2018 14:19

add admin check befor drop table, can not pass unit test because of b…

f16c839

…igint have admin check bug

remove some unit test to pass unit test

5bc7005

update test

c4b5d64

Merge branch 'master' of https://github.com/pingcap/tidb into add_adm…

0bf3744

…in_check_before_drop

shenli reviewed Aug 9, 2018

View reviewed changes

crazycs520 added 2 commits August 10, 2018 00:48

add test

1d51c1e

add log

169e122

pingcap deleted a comment from crazycs520 Aug 10, 2018

winkyao reviewed Aug 10, 2018

View reviewed changes

shenli reviewed Aug 10, 2018

View reviewed changes

crazycs520 added the component/DDL-need-LGT3 label Aug 10, 2018

crazycs520 added status/WIP and removed status/WIP labels Aug 10, 2018

crazycs520 force-pushed the add_admin_check_before_drop branch from ca822df to 40f5584 Compare August 10, 2018 06:20

use new seesion for server info syncer use go build ldflags

2472f38

crazycs520 force-pushed the add_admin_check_before_drop branch from 40f5584 to 2472f38 Compare August 10, 2018 06:21

winkyao reviewed Aug 10, 2018

View reviewed changes

alivxxx reviewed Aug 10, 2018

View reviewed changes

jackysp reviewed Aug 10, 2018

View reviewed changes

crazycs520 changed the title ~~ddl: Add admin check before drop table uses OS environment variable.~~ ddl: Add admin check before drop table useing building flag. Aug 10, 2018

jackysp reviewed Aug 10, 2018

View reviewed changes

crazycs520 added 2 commits August 10, 2018 17:33

atomic store and address comment

4c1d8df

add test for admin check

c8a25c3

winkyao reviewed Aug 10, 2018

View reviewed changes

crazycs520 added 2 commits August 11, 2018 14:41

update test

a5999d0

add admin check ldflags to go test

98fcf11

crazycs520 closed this Aug 11, 2018

crazycs520 reopened this Aug 11, 2018

shenli reviewed Aug 11, 2018

View reviewed changes

jackysp reviewed Aug 11, 2018

View reviewed changes

address comment

3a483e4

jackysp reviewed Aug 13, 2018

View reviewed changes

tiancaiamao reviewed Aug 13, 2018

View reviewed changes

crazycs520 added 2 commits August 13, 2018 11:12

address comment

53805ce

Merge branch 'master' of https://github.com/pingcap/tidb into add_adm…

f150a4b

…in_check_before_drop

crazycs520 force-pushed the add_admin_check_before_drop branch from 49a5c68 to f150a4b Compare August 13, 2018 06:41

crazycs520 added status/LGT2 Indicates that a PR has LGTM 2. status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Aug 14, 2018

ngaut changed the title ~~ddl: Add admin check before drop table useing building flag.~~ ddl: Add admin check before drop table using building flag. Aug 14, 2018

tiancaiamao approved these changes Aug 14, 2018

View reviewed changes

Merge branch 'master' into add_admin_check_before_drop

df19508

tiancaiamao merged commit 5404e2e into pingcap:master Aug 14, 2018

you06 added the sig/sql-infra SIG: SQL Infra label Mar 4, 2020

Comments

Conversation

crazycs520 commented Aug 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What have you changed? (mandatory)

Does this PR affect tidb-ansible update? (mandatory)

Does this PR need to be added to the release notes? (mandatory)

Refer to a related PR or issue link (optional)

Benchmark result if necessary (optional)

Add a few positive/negative examples (optional)

Uh oh!

crazycs520 commented Aug 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

winkyao left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

winkyao commented Aug 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jackysp commented Aug 10, 2018

Uh oh!

zz-jason commented Aug 10, 2018

Uh oh!

crazycs520 commented Aug 10, 2018

Uh oh!

winkyao commented Aug 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jackysp Aug 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

winkyao left a comment

Choose a reason for hiding this comment

Uh oh!

crazycs520 commented Aug 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jackysp commented Aug 13, 2018

crazycs520 commented Aug 9, 2018 •

edited

Loading

winkyao left a comment •

edited

Loading

winkyao commented Aug 10, 2018 •

edited

Loading

jackysp Aug 10, 2018 •

edited

Loading