*: add function flashback database #30217

erwadba · 2021-11-29T07:53:32Z

What problem does this PR solve?

Issue Number: close #20463

Problem Summary:

What is changed and how it works?

Add RecoverSchemaInfo struct contains SchemaInfo and TableInfo.
Add recoverSchema code logical like the recovertable .

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)

mysql> drop database m;
Query OK, 0 rows affected (0.43 sec)
mysql> create database m;
Query OK, 0 rows affected (0.20 sec)
mysql> use m;
Database changed
mysql> CREATE TABLE t(id int PRIMARY KEY AUTO_INCREMENT, c int);
Query OK, 0 rows affected (0.19 sec)
mysql> INSERT INTO t(c) VALUES (1);
Query OK, 1 row affected (0.03 sec)
mysql> INSERT INTO t(c) VALUES (2);
Query OK, 1 row affected (0.01 sec)
mysql> drop database m;
Query OK, 0 rows affected (0.42 sec)
mysql> flashback database m;
Query OK, 0 rows affected (0.97 sec)
mysql> use m ;
Database changed
mysql> select * from t;
+----+------+
| id | c    |
+----+------+
|  1 |    1 |
|  2 |    2 |
+----+------+
2 rows in set (0.01 sec)
mysql>

Side effects

N/A

Documentation

N/A

Release note

Add support `flashback database` command.

ti-chi-bot · 2021-11-29T07:53:33Z

[REVIEW NOTIFICATION]

This pull request has not been approved.

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

tangenta · 2021-11-29T12:09:44Z

/cc @crazycs520

bb7133 · 2021-12-01T09:26:04Z

ddl/ddl_api.go

+ Args: []interface{}{recoverSchemaInfo.SchemaInfo, recoverSchemaInfo.RecoversInfo,
+ recoverSchemaInfo.DropJobID, recoverSchemaInfo.SnapshotTS, recoverSchemaInfo.OldSchemaName},
+ }
+ err = d.doDDLJob(ctx, job)


Please handle the error here.

no need to handle error in here?

@erwadba Please address this comment

executor/ddl.go

bb7133 · 2021-12-01T09:31:52Z

Please add some more test cases for your feature.

bb7133 · 2021-12-01T09:34:19Z

Please fix the lint checking(https://ci.pingcap.net/blue/organizations/jenkins/tidb_ghpr_check/detail/tidb_ghpr_check/36907/pipeline):

[2021-11-30T01:45:41.313Z] ddl/schema.go:312:17: Error return value is not checked (errcheck)
[2021-11-30T01:45:41.313Z] 		defer enableGC(w)
[2021-11-30T01:45:41.313Z] 		              ^
[2021-11-30T01:45:41.313Z] ddl/schema.go:346:9: ineffectual assignment to err (ineffassign)
[2021-11-30T01:45:41.313Z] 			ver, err = updateVersionAndTableInfo(t, job, recoverInfo.TableInfo, false)
[2021-11-30T01:45:41.313Z] 			     ^
[2021-11-30T01:45:41.313Z] level=info msg="File cache stats: 1341 entries of total size 21.8MiB"
[2021-11-30T01:45:41.313Z] level=info msg="Memory: 1578 samples, avg is 2342.1MB, max is 9654.6MB"
[2021-11-30T01:45:41.313Z] level=info msg="Execution took 2m43.233059097s"

erwadba · 2021-12-03T07:56:47Z

/cc @bb7133 @crazycs520
PTAL

erwadba · 2021-12-06T02:16:27Z

/cc @bb7133 @crazycs520
PTAL

bb7133 · 2021-12-10T03:12:56Z

@erwadba Sorry that I missed this PR before, I will try to review it recently.

easonn7 · 2021-12-10T03:54:56Z

LGTM，but this feature should be designed under BR system, so please add BR PM and reviewed with LGTM.

executor/ddl.go

ddl/schema.go

crazycs520 · 2021-12-13T13:38:30Z

ddl/schema.go

+ ver, err = updateVersionAndTableInfo(t, job, recoverInfo.TableInfo, false)
+ if err != nil {
+ return ver, errors.Trace(err)
+ }


Suggested change

ver, err = updateVersionAndTableInfo(t, job, recoverInfo.TableInfo, false)

if err != nil {

return ver, errors.Trace(err)

}

Should only update schema version once in this situation. the related schema change should be set in updateSchemaVersion, related with the comment: https://github.com/pingcap/tidb/pull/30217/files#r767778383

crazycs520 · 2021-12-13T13:40:04Z

ddl/schema.go

+ schemaInfo.State = model.StatePublic
+ err = t.UpdateDatabase(schemaInfo)
+ if err != nil {
+ return ver, errors.Trace(err)
+ }


Suggested change

schemaInfo.State = model.StatePublic

err = t.UpdateDatabase(schemaInfo)

if err != nil {

return ver, errors.Trace(err)

}

you can create database with public state, then no need update schema info here.

erwadba · 2021-12-18T01:21:41Z

/run-unit-test

erwadba · 2021-12-18T07:11:13Z

/run-unit-test

sre-bot · 2021-12-18T07:22:08Z

Code Coverage Details: https://codecov.io/github/pingcap/tidb/commit/6336add026a26e8cc598e191b3d0c936bc6584ab

parser/ast/ddl_test.go

ddl/ddl_api.go

ddl/ddl_worker.go

ddl/ddl_api.go

crazycs520 · 2021-12-23T14:21:06Z

infoschema/builder.go

+ tids, err = b.applyCreateTable(m, di, tblInfo.ID, allocs, diff.Type, tids)
+ if err != nil {
+ return errors.Trace(err)
+ }


also need applyPlacementUpdate for each table?

@erwadba Please check this comment.

@xhebox Could you give some advice?

crazycs520 · 2021-12-23T14:25:30Z

ddl/ddl_worker.go

@@ -1066,6 +1090,7 @@ func updateSchemaVersion(t *meta.Meta, job *model.Job) (int64, error) {
 diff.AffectedOpts = buildPlacementAffects(oldIDs, oldIDs)


cc @djshow832 I'm not sure whether ActionRecoverSchema ddl job need this to update placement rule cache, PTAL

@xhebox @lcwangchao PTAL

@lcwangchao Do we implement it in this PR or not? I think we could recover placement rules without much problem..

crazycs520

cc @djshow832 PTAL, some part about placement rule, I'm not familiar with it.

erwadba · 2021-12-30T02:34:32Z

/run-unit-test

ti-chi-bot · 2021-12-31T14:50:02Z

@erwadba: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

bb7133 · 2022-01-19T06:22:47Z

Hi @erwadba Could you please resolve the conflicts? Thanks.

erwadba · 2022-01-26T01:23:30Z

/cc @bb7133
PTAL

wjhuang2016

Please resolve all the comments

erwadba · 2022-01-26T09:56:19Z

/run-all-tests

erwadba · 2022-01-27T08:08:05Z

/cc @bb7133 @wjhuang2016
PTAL

wjhuang2016 · 2022-01-28T08:21:30Z

ddl/ddl_api.go

+ SchemaID: recoverSchemaInfo.SchemaInfo.ID,
+ Type: model.ActionRecoverSchema,
+ BinlogInfo: &model.HistoryInfo{},
+ Args: []interface{}{recoverSchemaInfo, recoverCheckFlagNone},
 }
 err = d.doDDLJob(ctx, job)


Suggested change

err = d.doDDLJob(ctx, job)

err = d.doDDLJob(ctx, job)

if err != nil {

return err

}

ti-chi-bot · 2022-01-28T21:55:03Z

@erwadba: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

bb7133 · 2022-08-26T09:00:00Z

Hi @erwadba , this PR has been closed(due to a bug of Github, I believe), would you like to file another PR and we could work on it?

erwadba · 2022-08-29T07:04:53Z

Too many files conflicts,open another pr #37438. Another problem is here when exec flashback #37386

ti-chi-bot requested a review from crazycs520 November 29, 2021 12:09

bb7133 added sig/sql-infra SIG: SQL Infra type/new-feature require-LGT3 Indicates that the PR requires three LGTM. labels Dec 1, 2021

bb7133 reviewed Dec 1, 2021

View reviewed changes

executor/ddl.go Outdated Show resolved Hide resolved

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 1, 2021

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 2, 2021

erwadba force-pushed the add_flashback_database branch from c2ba7f3 to 068b3f0 Compare December 3, 2021 06:59

ti-chi-bot requested a review from bb7133 December 3, 2021 07:56

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 9, 2021