From 33f293157e765d1cb7207b139b6fc96578029ec1 Mon Sep 17 00:00:00 2001 From: Leavrth Date: Tue, 28 Nov 2023 11:06:41 +0800 Subject: [PATCH 1/5] remove duplicate bind info after snapshot restore Signed-off-by: Leavrth --- br/pkg/restore/systable_restore.go | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/br/pkg/restore/systable_restore.go b/br/pkg/restore/systable_restore.go index 409a4243c7121..6009075c48b9f 100644 --- a/br/pkg/restore/systable_restore.go +++ b/br/pkg/restore/systable_restore.go @@ -12,6 +12,7 @@ import ( berrors "github.com/pingcap/tidb/br/pkg/errors" "github.com/pingcap/tidb/br/pkg/logutil" "github.com/pingcap/tidb/br/pkg/utils" + "github.com/pingcap/tidb/pkg/bindinfo" "github.com/pingcap/tidb/pkg/parser/model" "github.com/pingcap/tidb/pkg/parser/mysql" filter "github.com/pingcap/tidb/pkg/util/table-filter" @@ -183,7 +184,7 @@ func (rc *Client) RestoreSystemSchemas(ctx context.Context, f filter.Filter) { tablesRestored = append(tablesRestored, tableName.L) } } - if err := rc.afterSystemTablesReplaced(tablesRestored); err != nil { + if err := rc.afterSystemTablesReplaced(ctx, tablesRestored); err != nil { for _, e := range multierr.Errors(err) { log.Warn("error during reconfigurating the system tables", zap.String("database", sysDB), logutil.ShortError(e)) } @@ -218,13 +219,13 @@ func (rc *Client) getDatabaseByName(name string) (*database, bool) { // afterSystemTablesReplaced do some extra work for special system tables. // e.g. after inserting to the table mysql.user, we must execute `FLUSH PRIVILEGES` to allow it take effect. -func (rc *Client) afterSystemTablesReplaced(tables []string) error { +func (rc *Client) afterSystemTablesReplaced(ctx context.Context, tables []string) error { var err error for _, table := range tables { if table == "user" { if rc.fullClusterRestore { log.Info("privilege system table restored, please reconnect to make it effective") - err = rc.dom.NotifyUpdatePrivilege() + err = multierr.Append(err, rc.dom.NotifyUpdatePrivilege()) } else { // to make it compatible with older version // todo: should we allow restore system table in non-fresh cluster in later br version? @@ -232,6 +233,14 @@ func (rc *Client) afterSystemTablesReplaced(tables []string) error { err = multierr.Append(err, errors.Annotatef(berrors.ErrUnsupportedSystemTable, "restored user info may not take effect, until you should execute `FLUSH PRIVILEGES` manually")) } + } else if table == "bind_info" { + if serr := rc.db.se.Execute(ctx, bindinfo.StmtRemoveDuplicatedPseudoBinding); serr != nil { + log.Warn("failed to delete duplicated pseudo binding", zap.Error(err)) + err = multierr.Append(err, + berrors.ErrUnknown.Wrap(err).GenWithStack("failed to delete duplicated pseudo binding %s", bindinfo.StmtRemoveDuplicatedPseudoBinding)) + } else { + log.Info("success to remove duplicated pseudo binding") + } } } return err From 3d04df0edce14f38a24a1c79b0e9e0482de95ef5 Mon Sep 17 00:00:00 2001 From: Leavrth Date: Tue, 28 Nov 2023 11:36:27 +0800 Subject: [PATCH 2/5] make bazel happy Signed-off-by: Leavrth --- br/pkg/restore/BUILD.bazel | 1 + 1 file changed, 1 insertion(+) diff --git a/br/pkg/restore/BUILD.bazel b/br/pkg/restore/BUILD.bazel index 02b2fb047063d..12cc400eee19a 100644 --- a/br/pkg/restore/BUILD.bazel +++ b/br/pkg/restore/BUILD.bazel @@ -48,6 +48,7 @@ go_library( "//br/pkg/utils/iter", "//br/pkg/utils/storewatch", "//br/pkg/version", + "//pkg/bindinfo", "//pkg/config", "//pkg/ddl", "//pkg/ddl/util", From f7474aa7678dfcf69f5396585df5d4f024da2938 Mon Sep 17 00:00:00 2001 From: Leavrth Date: Tue, 28 Nov 2023 13:20:14 +0800 Subject: [PATCH 3/5] expose the error when restore system table Signed-off-by: Leavrth --- br/pkg/restore/systable_restore.go | 16 +++++++--------- br/pkg/task/restore.go | 5 ++++- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/br/pkg/restore/systable_restore.go b/br/pkg/restore/systable_restore.go index 6009075c48b9f..59463bce5a366 100644 --- a/br/pkg/restore/systable_restore.go +++ b/br/pkg/restore/systable_restore.go @@ -149,7 +149,7 @@ func (rc *Client) ClearSystemUsers(ctx context.Context, resetUsers []string) err // RestoreSystemSchemas restores the system schema(i.e. the `mysql` schema). // Detail see https://github.com/pingcap/br/issues/679#issuecomment-762592254. -func (rc *Client) RestoreSystemSchemas(ctx context.Context, f filter.Filter) { +func (rc *Client) RestoreSystemSchemas(ctx context.Context, f filter.Filter) error { sysDB := mysql.SystemDB temporaryDB := utils.TemporaryDBName(sysDB) @@ -157,18 +157,18 @@ func (rc *Client) RestoreSystemSchemas(ctx context.Context, f filter.Filter) { if !f.MatchSchema(sysDB) || !rc.withSysTable { log.Debug("system database filtered out", zap.String("database", sysDB)) - return + return nil } originDatabase, ok := rc.databases[temporaryDB.O] if !ok { log.Info("system database not backed up, skipping", zap.String("database", sysDB)) - return + return nil } db, ok := rc.getDatabaseByName(sysDB) if !ok { // Or should we create the database here? log.Warn("target database not exist, aborting", zap.String("database", sysDB)) - return + return nil } tablesRestored := make([]string, 0, len(originDatabase.Tables)) @@ -180,15 +180,13 @@ func (rc *Client) RestoreSystemSchemas(ctx context.Context, f filter.Filter) { logutil.ShortError(err), zap.Stringer("table", tableName), ) + return errors.Trace(err) } tablesRestored = append(tablesRestored, tableName.L) } } - if err := rc.afterSystemTablesReplaced(ctx, tablesRestored); err != nil { - for _, e := range multierr.Errors(err) { - log.Warn("error during reconfigurating the system tables", zap.String("database", sysDB), logutil.ShortError(e)) - } - } + err := rc.afterSystemTablesReplaced(ctx, tablesRestored) + return errors.Trace(err) } // database is a record of a database. diff --git a/br/pkg/task/restore.go b/br/pkg/task/restore.go index a890ce746f93e..0de103b98fef0 100644 --- a/br/pkg/task/restore.go +++ b/br/pkg/task/restore.go @@ -1038,7 +1038,10 @@ func runRestore(c context.Context, g glue.Glue, cmdName string, cfg *RestoreConf // The cost of rename user table / replace into system table wouldn't be so high. // So leave it out of the pipeline for easier implementation. - client.RestoreSystemSchemas(ctx, cfg.TableFilter) + err = client.RestoreSystemSchemas(ctx, cfg.TableFilter) + if err != nil { + return errors.Trace(err) + } schedulersRemovable = true From 7ead7cdf98e7f2b6d3a89710b9f531f9560752b9 Mon Sep 17 00:00:00 2001 From: Leavrth Date: Tue, 28 Nov 2023 13:25:25 +0800 Subject: [PATCH 4/5] replace the error Signed-off-by: Leavrth --- br/pkg/restore/systable_restore.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/br/pkg/restore/systable_restore.go b/br/pkg/restore/systable_restore.go index 59463bce5a366..7e629b33e0e50 100644 --- a/br/pkg/restore/systable_restore.go +++ b/br/pkg/restore/systable_restore.go @@ -233,9 +233,9 @@ func (rc *Client) afterSystemTablesReplaced(ctx context.Context, tables []string } } else if table == "bind_info" { if serr := rc.db.se.Execute(ctx, bindinfo.StmtRemoveDuplicatedPseudoBinding); serr != nil { - log.Warn("failed to delete duplicated pseudo binding", zap.Error(err)) + log.Warn("failed to delete duplicated pseudo binding", zap.Error(serr)) err = multierr.Append(err, - berrors.ErrUnknown.Wrap(err).GenWithStack("failed to delete duplicated pseudo binding %s", bindinfo.StmtRemoveDuplicatedPseudoBinding)) + berrors.ErrUnknown.Wrap(serr).GenWithStack("failed to delete duplicated pseudo binding %s", bindinfo.StmtRemoveDuplicatedPseudoBinding)) } else { log.Info("success to remove duplicated pseudo binding") } From d8a333f819f9194cc9282b4beb7f1162165b406f Mon Sep 17 00:00:00 2001 From: Leavrth Date: Fri, 8 Dec 2023 16:06:14 +0800 Subject: [PATCH 5/5] annotate the error Signed-off-by: Leavrth --- br/pkg/restore/systable_restore.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/br/pkg/restore/systable_restore.go b/br/pkg/restore/systable_restore.go index 7e629b33e0e50..29ab3213cd695 100644 --- a/br/pkg/restore/systable_restore.go +++ b/br/pkg/restore/systable_restore.go @@ -180,13 +180,15 @@ func (rc *Client) RestoreSystemSchemas(ctx context.Context, f filter.Filter) err logutil.ShortError(err), zap.Stringer("table", tableName), ) - return errors.Trace(err) + return errors.Annotatef(err, "error during merging temporary tables into system tables, table: %s", tableName) } tablesRestored = append(tablesRestored, tableName.L) } } - err := rc.afterSystemTablesReplaced(ctx, tablesRestored) - return errors.Trace(err) + if err := rc.afterSystemTablesReplaced(ctx, tablesRestored); err != nil { + return errors.Annotate(err, "error during extra works after system tables replaced") + } + return nil } // database is a record of a database.