br: error if the log restore has no full backup schema or id maps #54421

Leavrth · 2024-07-03T10:32:39Z

What problem does this PR solve?

Issue Number: close #54418

Problem Summary:
Performing full restore and log restore separately will be a disaster

What changed and how does it work?

output error if the log restore has no full backup schema or id maps

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No need to test
- I checked and no code files have been changed.

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.

Prevent the cluster from performing a PITR recovery for the first time without specifying --full-backup-storage.

Signed-off-by: Jianjun Liao <[email protected]>

tiprow · 2024-07-03T10:32:58Z

Hi @Leavrth. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

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-sigs/prow repository.

codecov · 2024-07-03T10:51:17Z

Codecov Report

Attention: Patch coverage is 46.93878% with 26 lines in your changes missing coverage. Please review.

Project coverage is 76.0048%. Comparing base (da7ed5c) to head (a38b874).
Report is 17 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #54421        +/-   ##
================================================
+ Coverage   74.8082%   76.0048%   +1.1966%     
================================================
  Files          1563       1584        +21     
  Lines        364342     475280    +110938     
================================================
+ Hits         272558     361236     +88678     
- Misses        72088      92246     +20158     
- Partials      19696      21798      +2102

Flag	Coverage Δ
integration	`51.1511% <46.9387%> (?)`
unit	`71.9319% <0.0000%> (-1.7651%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
dumpling	`52.9567% <ø> (-2.2327%)`	⬇️
parser	`∅ <ø> (∅)`
br	`63.0078% <46.9387%> (+15.3012%)`	⬆️

kennytm · 2024-07-05T03:41:33Z

Hold on, will this make Full -> Incremental -> PITR (supply --start-ts only) impossible to run (assume incremental has no DDLs, and ignore the RewriteTS issue at the moment)

Leavrth · 2024-07-05T06:27:48Z

Hold on, will this make Full -> Incremental -> PITR (supply --start-ts only) impossible to run (assume incremental has no DDLs, and ignore the RewriteTS issue at the moment)

Yes, this PR prohibits the behavior even if there is no rewrite ts problem. The issue #54418 provides an example.

kennytm · 2024-07-05T17:31:38Z

@Leavrth is the --start-ts flag still usable?

(anyway i'm tagging release-note this is behavior-changing.)

Leavrth · 2024-07-08T04:14:42Z

@Leavrth is the --start-ts flag still usable?

(anyway i'm tagging release-note this is behavior-changing.)

Yes. Currently log restore will upload a file to the external storage, which contains id-maps from the upstream table/db ids to the pre-allocated downstream table/db ids at restored-ts.
This path of id-maps is:

tidb/br/pkg/metautil/metafile.go

Line 62 in 7a0611f

    
           return fmt.Sprintf("%s/pitr_id_map.cluster_id:%d.restored_ts:%d", "pitr_id_maps", clusterID, restoreTS)

With the id-maps,

log restore can retry with checkpoint if it failed.

if log restore with --restored-ts T1 is finished. The next log restore with start-ts T1 but without full-backup-storage will read the id-maps pitr_id_maps/pitr_id_map.cluster_id:{cluster-id}.restored_ts:{start-ts} instead of reading the tableinfos in backupmeta.

tidb/br/pkg/restore/log_client/client.go

Lines 674 to 693 in 7a0611f

    
           // a new task, but without full snapshot restore, tries to load 
        
           // schemas map whose `restore-ts`` is the task's `start-ts`. 
        
           if len(dbMaps) <= 0 && !cfg.HasFullRestore { 
        
           	log.Info("try to load pitr id maps of the previous task", zap.Uint64("start-ts", rc.startTS)) 
        
           	needConstructIdMap = true 
        
           	dbMaps, err = rc.initSchemasMap(ctx, rc.GetClusterID(ctx), rc.startTS) 
        
           	if err != nil { 
        
           		return nil, errors.Trace(err) 
        
           	} 
        
           	info := rc.dom.InfoSchema() 
        
           	shcemas := info.AllSchemaNames() 
        
           	for _, schema := range shcemas { 
        
           		for _, table := range info.SchemaTables(schema) { 
        
           			tableInfo := table.Meta() 
        
           			if tableInfo.TiFlashReplica != nil && tableInfo.TiFlashReplica.Count > 0 { 
        
           				return nil, errors.Errorf("exist table(s) have tiflash replica, please remove it before restore") 
        
           			} 
        
           		} 
        
           	} 
        
           }

kennytm · 2024-07-08T12:33:44Z

it seems id-map was introduced in v7.1.0, so --start-ts requires an archive made from v7.1 at least I guess.

BornChanger · 2024-07-24T01:48:35Z

/retest

tiprow · 2024-07-24T01:49:01Z

@BornChanger: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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-sigs/prow repository.

Leavrth · 2024-07-24T01:55:28Z

it seems id-map was introduced in v7.1.0, so --start-ts requires an archive made from v7.1 at least I guess.

In v6.5, BR will upload a new backupmeta_restored-ts with latest table information after log restore done, but I have not tested it.

tidb/br/pkg/task/stream.go

Lines 1310 to 1312 in e07d004

    
           if err = client.SaveSchemas(ctx, schemasReplace, logMinTS, cfg.RestoreTS); err != nil { 
        
           	return errors.Trace(err) 
        
           }

Leavrth · 2024-08-01T08:13:36Z

/retest

tiprow · 2024-08-01T08:16:46Z

@Leavrth: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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-sigs/prow repository.

Signed-off-by: Jianjun Liao <[email protected]>

YuJuncen

rest lgtm

YuJuncen · 2024-08-02T10:10:24Z

br/pkg/restore/log_client/client.go

+) (map[stream.UpstreamID]*stream.DBReplace, error) {
+	dbReplaces := make(map[stream.UpstreamID]*stream.DBReplace)
+	if cfg.FullBackupStorage == nil {
+		envVal, ok := os.LookupEnv("UNSAFE_PITR_LOG_RESTORE_START_BEFORE_ANY_UPSTREAM_USER_DDL")


Maybe extract it to a constant?

Signed-off-by: Jianjun Liao <[email protected]>

ti-chi-bot · 2024-08-05T02:42:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 3pointer, YuJuncen

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~br/OWNERS~~ [3pointer,YuJuncen]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ti-chi-bot · 2024-08-05T02:42:59Z

[LGTM Timeline notifier]

Timeline:

2024-08-02 08:34:54.714862645 +0000 UTC m=+78491.933624238: ☑️ agreed by 3pointer.
2024-08-05 02:42:58.738734736 +0000 UTC m=+233508.605833831: ☑️ agreed by YuJuncen.

Leavrth · 2024-08-05T03:54:44Z

/retest

tiprow · 2024-08-05T03:55:38Z

@Leavrth: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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-sigs/prow repository.

Leavrth · 2024-08-05T03:59:51Z

/ok-to-test

Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot · 2024-08-05T04:29:56Z

In response to a cherrypick label: new pull request created to branch release-6.5: #55180.

ti-chi-bot · 2024-08-05T04:30:39Z

In response to a cherrypick label: new pull request created to branch release-7.1: #55181.

Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot · 2024-08-05T04:31:20Z

In response to a cherrypick label: new pull request created to branch release-7.5: #55182.

Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot · 2024-08-05T04:32:01Z

In response to a cherrypick label: new pull request created to branch release-8.1: #55183.

Signed-off-by: ti-chi-bot <[email protected]>

ref #54418

error if the log restore has no full backup schema or id maps

9da44d3

Signed-off-by: Jianjun Liao <[email protected]>

ti-chi-bot bot added do-not-merge/needs-tests-checked release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/needs-triage-completed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed do-not-merge/needs-tests-checked labels Jul 3, 2024

ti-chi-bot bot removed the do-not-merge/needs-triage-completed label Jul 3, 2024

Leavrth mentioned this pull request Jul 5, 2024

BR compatible with InfoSchemaV2 #52717

Closed

kennytm added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jul 5, 2024

ti-chi-bot bot removed the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jul 5, 2024

ti-chi-bot bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Jul 8, 2024

Leavrth added 2 commits August 1, 2024 17:37

Merge branch 'master' into restore_check

20b8b3b

remove unnecessary config item

7b03cc6

Signed-off-by: Jianjun Liao <[email protected]>

introduce a environment to skip loading the base schemas

f22d950

Signed-off-by: Jianjun Liao <[email protected]>

ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 2, 2024

bazel build

788e481

Signed-off-by: Jianjun Liao <[email protected]>

YuJuncen reviewed Aug 2, 2024

View reviewed changes

Leavrth added 2 commits August 2, 2024 18:25

commit some suggestions

efc81a2

Signed-off-by: Jianjun Liao <[email protected]>

commit some suggestions

a38b874

Signed-off-by: Jianjun Liao <[email protected]>

YuJuncen approved these changes Aug 5, 2024

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Aug 5, 2024

ti-chi-bot bot added the ok-to-test Indicates a PR is ready to be tested. label Aug 5, 2024

ti-chi-bot bot merged commit ce45eff into pingcap:master Aug 5, 2024
32 checks passed

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Aug 5, 2024

This is an automated cherry-pick of pingcap#54421

7ec7bd9

Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot mentioned this pull request Aug 5, 2024

br: error if the log restore has no full backup schema or id maps (#54421) #55180

Open

13 tasks

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Aug 5, 2024

This is an automated cherry-pick of pingcap#54421

b79e351

Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot mentioned this pull request Aug 5, 2024

br: error if the log restore has no full backup schema or id maps (#54421) #55181

Open

13 tasks

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Aug 5, 2024

This is an automated cherry-pick of pingcap#54421

6babf90

Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot mentioned this pull request Aug 5, 2024

br: error if the log restore has no full backup schema or id maps (#54421) #55182

Open

13 tasks

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Aug 5, 2024

This is an automated cherry-pick of pingcap#54421

bdeb703

Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot mentioned this pull request Aug 5, 2024

br: error if the log restore has no full backup schema or id maps (#54421) #55183

Open

13 tasks

ti-chi-bot bot pushed a commit that referenced this pull request Aug 27, 2024

br: add test for #54421 (#55571)

290dc46

ref #54418

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

br: error if the log restore has no full backup schema or id maps #54421

br: error if the log restore has no full backup schema or id maps #54421

Leavrth commented Jul 3, 2024 •

edited

Loading

tiprow bot commented Jul 3, 2024

codecov bot commented Jul 3, 2024 •

edited

Loading

kennytm commented Jul 5, 2024

Leavrth commented Jul 5, 2024

kennytm commented Jul 5, 2024 •

edited

Loading

Leavrth commented Jul 8, 2024 •

edited

Loading

kennytm commented Jul 8, 2024

BornChanger commented Jul 24, 2024

tiprow bot commented Jul 24, 2024

Leavrth commented Jul 24, 2024

Leavrth commented Aug 1, 2024

tiprow bot commented Aug 1, 2024

YuJuncen left a comment

YuJuncen Aug 2, 2024

Leavrth Aug 2, 2024

ti-chi-bot bot commented Aug 5, 2024

ti-chi-bot bot commented Aug 5, 2024

Leavrth commented Aug 5, 2024

tiprow bot commented Aug 5, 2024

Leavrth commented Aug 5, 2024

ti-chi-bot commented Aug 5, 2024

ti-chi-bot commented Aug 5, 2024

ti-chi-bot commented Aug 5, 2024

ti-chi-bot commented Aug 5, 2024

br: error if the log restore has no full backup schema or id maps #54421

br: error if the log restore has no full backup schema or id maps #54421

Conversation

Leavrth commented Jul 3, 2024 • edited Loading

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

tiprow bot commented Jul 3, 2024

codecov bot commented Jul 3, 2024 • edited Loading

Codecov Report

kennytm commented Jul 5, 2024

Leavrth commented Jul 5, 2024

kennytm commented Jul 5, 2024 • edited Loading

Leavrth commented Jul 8, 2024 • edited Loading

kennytm commented Jul 8, 2024

BornChanger commented Jul 24, 2024

tiprow bot commented Jul 24, 2024

Leavrth commented Jul 24, 2024

Leavrth commented Aug 1, 2024

tiprow bot commented Aug 1, 2024

YuJuncen left a comment

Choose a reason for hiding this comment

YuJuncen Aug 2, 2024

Choose a reason for hiding this comment

Leavrth Aug 2, 2024

Choose a reason for hiding this comment

ti-chi-bot bot commented Aug 5, 2024

ti-chi-bot bot commented Aug 5, 2024

[LGTM Timeline notifier]

Leavrth commented Aug 5, 2024

tiprow bot commented Aug 5, 2024

Leavrth commented Aug 5, 2024

ti-chi-bot commented Aug 5, 2024

ti-chi-bot commented Aug 5, 2024

ti-chi-bot commented Aug 5, 2024

ti-chi-bot commented Aug 5, 2024

Leavrth commented Jul 3, 2024 •

edited

Loading

codecov bot commented Jul 3, 2024 •

edited

Loading

kennytm commented Jul 5, 2024 •

edited

Loading

Leavrth commented Jul 8, 2024 •

edited

Loading