br: pipeline backup schemas #43003

Leavrth · 2023-04-12T12:50:16Z

What problem does this PR solve?

Issue Number: close #43002

Problem Summary:
when backup a large cluster, br will loads many table info into memory, which leads to oom.

What is changed and how it works?

pipeline backup schemas

Check List

Tests

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

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

Signed-off-by: Leavrth <jianjun.liao@outlook.com>

ti-chi-bot · 2023-04-12T12:50:18Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

BornChanger
YuJuncen

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.

Signed-off-by: Leavrth <jianjun.liao@outlook.com>

Leavrth · 2023-04-14T05:19:58Z

br/pkg/backup/client.go

+		ranges, schemas, policies, err := BuildBackupRangeAndSchema(storage, tableFilter, backupTS, isFullBackup, true)
+		if err != nil {
+			return nil, nil, nil, errors.Trace(err)
+		}
+		// Add keyspace prefix to BackupRequest
+		for i := range ranges {
+			start, end := ranges[i].StartKey, ranges[i].EndKey
+			ranges[i].StartKey, ranges[i].EndKey = storage.GetCodec().EncodeRange(start, end)
+		}
+		return ranges, schemas, policies, err


Note: change the ranges with keyspace prefix only when get the ranges from kv storage instead of checkpoint data.

Leavrth · 2023-04-14T05:20:27Z

br/pkg/backup/client.go

@@ -498,7 +507,7 @@ func BuildBackupRangeAndSchema(
 	backupTS uint64,
 	isFullBackup bool,
 	buildRange bool,
-) ([]rtree.Range, *Schemas, []*backuppb.PlacementPolicy, error) {
+) ([]rtree.Range, *SchemasV2, []*backuppb.PlacementPolicy, error) {


Note: only records the size of the schemas for progress in this function

Leavrth · 2023-04-14T05:23:17Z

br/pkg/backup/client.go

 		if err != nil {
 			return nil, nil, nil, errors.Trace(err)
 		}

-		if len(tables) == 0 {


Note: the origin code might have a little mistake: When len(tables) > 0 but all the tables is skipped by table-filter, so the dbInfo won't be added into schemas

YuJuncen

Generally LGTM, is it possible to remove the old Schemas BTW?

br/pkg/backup/schema.go

Signed-off-by: Leavrth <jianjun.liao@outlook.com>

br/pkg/backup/client.go

BornChanger · 2023-04-15T14:45:08Z

br/pkg/backup/client.go

+				return nil
+			}
+
+			schemasNum += 1


Why schemaNum increasing at the same frequency as tableNum?

schemaNum is to record the total number of (dbInfo, tableInfo) or (dbInfo, nil). (equivalent to schemas.Len()).
tableNum is to record whether the dbInfo has records any tableInfos. (equivalent to len(tableInfos)).

now tableNum has been changed to var hasTable bool

br/pkg/restore/client.go

BornChanger · 2023-04-15T14:48:40Z

br/pkg/task/stream.go

 	err = schemas.BackupSchemas(ctx, metaWriter, nil, s.mgr.GetStorage(), nil,
-		s.cfg.StartTS, schemasConcurrency, 0, true, nil)
+		s.cfg.StartTS, backup.DefaultSchemaConcurrency, 0, true, nil)


Concurrency is different now. Is it intentional?

That's because the new schemas need to iterates all tables to get the size, which is repeated in this situation. Therefore, we don't get the size of schemas.

Besides, we can see the workerpool in schemas.BackupSchemas only push the worker to channel. Only when call ApplyOnErrorGroup, it would create a new goroutine. So directly use backup.DefaultSchemaConcurrency is somewhat equivalent to mathutil.Min(backup.DefaultSchemaConcurrency, schemas.Len())).

If schemas.Len() >= backup.DefaultSchemaConcurrency, it is the same to use backup.DefaultSchemaConcurrency.
If schemas.Len() < backup.DefaultSchemaConcurrency, the new version would create the schemas.Len() goroutine in total, which is the same as old version. And backup.DefaultSchemaConcurrency - schemas.Len() more worker struct add into the channel, which is no effect.

Signed-off-by: Leavrth <jianjun.liao@outlook.com>

Leavrth · 2023-04-17T06:49:15Z

/merge

ti-chi-bot · 2023-04-17T06:49:19Z

This pull request has been accepted and is ready to merge.

Commit hash: e0e19e5

Leavrth · 2023-04-17T06:49:32Z

/retest

Leavrth · 2023-04-17T07:37:56Z

/retest

Signed-off-by: Leavrth <jianjun.liao@outlook.com>

Leavrth · 2023-04-17T11:04:15Z

/retest

Leavrth · 2023-04-17T11:08:24Z

/merge

ti-chi-bot · 2023-04-17T11:08:28Z

This pull request has been accepted and is ready to merge.

Commit hash: 373705f

Leavrth · 2023-04-17T12:50:33Z

/retest

Leavrth · 2023-04-17T13:40:25Z

/retest

Leavrth added 2 commits April 10, 2023 17:53

draft

a789d69

Signed-off-by: Leavrth <jianjun.liao@outlook.com>

add unit test

27528d4

Signed-off-by: Leavrth <jianjun.liao@outlook.com>

ti-chi-bot added release-note-none do-not-merge/needs-triage-completed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 12, 2023

Leavrth added 2 commits April 14, 2023 10:50

Merge branch 'master' into pipeline_backup

f24c365

fix ranges change issue

a7a3910

Signed-off-by: Leavrth <jianjun.liao@outlook.com>

ti-chi-bot removed the do-not-merge/needs-triage-completed label Apr 14, 2023

Leavrth commented Apr 14, 2023

View reviewed changes

YuJuncen approved these changes Apr 14, 2023

View reviewed changes

br/pkg/backup/schema.go Outdated Show resolved Hide resolved

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Apr 14, 2023

remove old schemas

d84dd9a

Signed-off-by: Leavrth <jianjun.liao@outlook.com>

BornChanger reviewed Apr 14, 2023

View reviewed changes

br/pkg/backup/client.go Outdated Show resolved Hide resolved

BornChanger reviewed Apr 14, 2023

View reviewed changes

br/pkg/backup/client.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 Apr 14, 2023

BornChanger reviewed Apr 15, 2023

View reviewed changes

br/pkg/restore/client.go Outdated Show resolved Hide resolved

BornChanger reviewed Apr 15, 2023

View reviewed changes

Leavrth added 2 commits April 15, 2023 23:16

Merge branch 'master' into pipeline_backup

44e6e85

commit some suggestions

e0e19e5

Signed-off-by: Leavrth <jianjun.liao@outlook.com>

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

BornChanger approved these changes Apr 17, 2023

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Apr 17, 2023

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Apr 17, 2023

fix unit test

373705f

Signed-off-by: Leavrth <jianjun.liao@outlook.com>

ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Apr 17, 2023

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Apr 17, 2023

ti-chi-bot merged commit 9cf0ed8 into pingcap:master Apr 17, 2023
11 checks passed

Leavrth mentioned this pull request Mar 11, 2024

br: reduce memory footprint and HWM (high water mark) #51573

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

br: pipeline backup schemas #43003

br: pipeline backup schemas #43003

Leavrth commented Apr 12, 2023 •

edited

ti-chi-bot commented Apr 12, 2023 •

edited

Leavrth Apr 14, 2023

Leavrth Apr 14, 2023

Leavrth Apr 14, 2023

YuJuncen left a comment

BornChanger Apr 15, 2023

Leavrth Apr 15, 2023 •

edited

BornChanger Apr 15, 2023

Leavrth Apr 15, 2023 •

edited

Leavrth commented Apr 17, 2023

ti-chi-bot commented Apr 17, 2023

Leavrth commented Apr 17, 2023

Leavrth commented Apr 17, 2023

Leavrth commented Apr 17, 2023

Leavrth commented Apr 17, 2023

ti-chi-bot commented Apr 17, 2023

Leavrth commented Apr 17, 2023

Leavrth commented Apr 17, 2023

br: pipeline backup schemas #43003

br: pipeline backup schemas #43003

Conversation

Leavrth commented Apr 12, 2023 • edited

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Apr 12, 2023 • edited

Leavrth Apr 14, 2023

Choose a reason for hiding this comment

Leavrth Apr 14, 2023

Choose a reason for hiding this comment

Leavrth Apr 14, 2023

Choose a reason for hiding this comment

YuJuncen left a comment

Choose a reason for hiding this comment

BornChanger Apr 15, 2023

Choose a reason for hiding this comment

Leavrth Apr 15, 2023 • edited

Choose a reason for hiding this comment

BornChanger Apr 15, 2023

Choose a reason for hiding this comment

Leavrth Apr 15, 2023 • edited

Choose a reason for hiding this comment

Leavrth commented Apr 17, 2023

ti-chi-bot commented Apr 17, 2023

Leavrth commented Apr 17, 2023

Leavrth commented Apr 17, 2023

Leavrth commented Apr 17, 2023

Leavrth commented Apr 17, 2023

ti-chi-bot commented Apr 17, 2023

Leavrth commented Apr 17, 2023

Leavrth commented Apr 17, 2023

Leavrth commented Apr 12, 2023 •

edited

ti-chi-bot commented Apr 12, 2023 •

edited

Leavrth Apr 15, 2023 •

edited

Leavrth Apr 15, 2023 •

edited