br: migrate pitr id map to the system table `mysql.tidb_pitr_id_map` #55871

Leavrth · 2024-09-05T06:01:22Z

What problem does this PR solve?

Issue Number: ref #55870

Problem Summary:
BR restore should not have the permission to write/delete the external storage.

What changed and how does it work?

migrate pitr id map to the system table mysql.tidb_pitr_id_map

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.

None

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

tiprow · 2024-09-05T06:01:40Z

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-09-05T06:17:56Z

Codecov Report

Attention: Patch coverage is 72.30769% with 18 lines in your changes missing coverage. Please review.

Project coverage is 56.8682%. Comparing base (bb3456b) to head (f78334d).
Report is 3 commits behind head on master.

Additional details and impacted files

@@                Coverage Diff                @@
##             master     #55871         +/-   ##
=================================================
- Coverage   72.9007%   56.8682%   -16.0326%     
=================================================
  Files          1604       1756        +152     
  Lines        446805     630412     +183607     
=================================================
+ Hits         325724     358504      +32780     
- Misses       101028     247249     +146221     
- Partials      20053      24659       +4606

Flag	Coverage Δ
integration	`39.6444% <27.6923%> (?)`
unit	`72.0391% <64.6153%> (+0.0320%)`	⬆️

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

Components	Coverage Δ
dumpling	`52.9567% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`63.1987% <74.1379%> (+17.4949%)`	⬆️

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

pkg/session/bootstrap.go

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

3pointer · 2024-09-09T10:01:29Z

br/pkg/restore/log_client/client.go

+		kv.WithInternalSourceType(ctx, kv.InternalTxnBR),
+		nil,
+		getPitrIDMapSQL,
+		restoreTS,


restoreTS maybe confusing, because sometimes we use start-ts to build pitr map. esspecially in L584.

The current log restore's start-ts is the last log restore's restored-ts, so BR gets the pitr id map at start-ts of the current log restore is actually to get the pitr id map at restored-ts of the last log restore.

3pointer · 2024-09-09T10:05:02Z

br/pkg/restore/log_client/client.go

+	if err != nil {
+		return errors.Trace(err)
+	}
+	// clean the dirty id map at first


is it safe? if process exits abnormally after deleting finished.

We have the processes to persist id map:

directly get the pitr id map if checkpoint task progress mark that BR has persist pitr id map.

if not, generate pitr id map, and
a. delete the id map at restored ts
b. write the id map at restored ts
c. update the checkpoint task progress to mark that BR has persist pitr id map.

In the external storage, step 2.a and 2.b is replaced with uploading files with override mode. Actually, the atomic is not necessary. That's because there must be failed at the step 2.b or 2.c, which means the pitr id map is incomplete. But just do the step 2.a in the next execution to delete the incomplete pitr id map.

Based on id map at start-ts and the metakv to generate the new id map at restored-ts. So delete the new id map at restored-ts is safe because we still can regenerate the same one.

YuJuncen · 2024-09-11T07:19:45Z

br/pkg/version/version.go

+	if BRVersion.Major > 8 || (BRVersion.Major == 8 && BRVersion.Minor >= 4) {
+		if tikvVersion.Major < 8 || (tikvVersion.Major == 8 && tikvVersion.Minor < 4) {
+			return errors.Annotatef(berrors.ErrVersionMismatch,
+				"TiKV node %s version %s is too old because the PITR id map is written into the cluster system table mysql.tidb_pitr_id_map, please use the tikv with version v8.4.0+",


I guess TiKV isn't related to the systable...? Perhaps replace "TiKV" with "cluster".

Yes, there seems an assumption that each component version is the same.

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

ti-chi-bot · 2024-09-12T03:34:10Z

[LGTM Timeline notifier]

Timeline:

2024-09-11 07:21:27.930943161 +0000 UTC m=+427357.671367100: ☑️ agreed by YuJuncen.
2024-09-12 03:34:09.829230921 +0000 UTC m=+500119.569654859: ☑️ agreed by 3pointer.

yudongusa

LGTM for the bootstrap.

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

lance6716

/approve

for import part

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

Leavrth · 2024-09-12T08:29:45Z

/ok-to-test

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

windtalker

LGTM

Leavrth · 2024-09-13T04:35:48Z

/retest

ti-chi-bot · 2024-09-13T06:40:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 3pointer, hawkingrei, lance6716, windtalker, winoros, yudongusa, 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:

~~OWNERS~~ [3pointer,YuJuncen,hawkingrei,lance6716,windtalker,winoros]
~~br/OWNERS~~ [3pointer,YuJuncen]
~~pkg/ddl/OWNERS~~ [lance6716]
~~pkg/executor/importer/OWNERS~~ [lance6716]
~~pkg/expression/OWNERS~~ [windtalker]
~~pkg/session/OWNERS~~ [yudongusa]
~~pkg/statistics/OWNERS~~ [hawkingrei,winoros]

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

Leavrth added 4 commits September 4, 2024 11:38

draft

ea63cf4

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

draft

3764be3

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

bazel prepare

69fba50

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

Merge branch 'master' into pitr_id_map

70c2341

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 5, 2024

add unit test

5557409

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

Leavrth mentioned this pull request Sep 5, 2024

BR restore should not have the permission to write/delete the external storage. #55870

Open

2 tasks

Leavrth added 2 commits September 5, 2024 18:08

preallocate metaData

260cad6

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

add version check

e0ae93c

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

Leavrth commented Sep 6, 2024

View reviewed changes

pkg/session/bootstrap.go Outdated Show resolved Hide resolved

add upstream cluster id

2f35f29

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

ti-chi-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 9, 2024

Leavrth added 2 commits September 9, 2024 15:55

fix typos

9697e48

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

fix index

b115f48

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

3pointer reviewed Sep 9, 2024

View reviewed changes

YuJuncen approved these changes Sep 11, 2024

View reviewed changes

ti-chi-bot bot added the needs-1-more-lgtm label Sep 11, 2024

Leavrth added 2 commits September 11, 2024 18:46

fix unit test

5bdca09

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

fix unit test

5e80aa8

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

3pointer approved these changes Sep 12, 2024

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm labels Sep 12, 2024

yudongusa approved these changes Sep 12, 2024

View reviewed changes

fix unit test

f4d5dda

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

lance6716 reviewed Sep 12, 2024

View reviewed changes

ti-chi-bot bot added the approved label Sep 12, 2024

Leavrth added 3 commits September 12, 2024 14:06

fix unit test

b9d858e

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

fix unit test

d605dd8

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

fix unit test

b095fdb

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

ti-chi-bot bot added the ok-to-test label Sep 12, 2024

Leavrth mentioned this pull request Sep 12, 2024

br: update restore checkpoint information pingcap/docs-cn#18623

Open

16 tasks

Leavrth added 3 commits September 12, 2024 17:27

fix unit test

c6f6d7f

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

Merge branch 'master' into pitr_id_map

2cf1bd5

fix unit test

c82be88

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

ti-chi-bot bot removed the approved label Sep 12, 2024

fix unit test

fedd8e4

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

ti-chi-bot bot added the sig/planner SIG: Planner label Sep 12, 2024

fix unit test

8f39023

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

windtalker approved these changes Sep 13, 2024

View reviewed changes

Merge branch 'master' into pitr_id_map

f78334d

hawkingrei approved these changes Sep 13, 2024

View reviewed changes

ti-chi-bot bot added the approved label Sep 13, 2024

winoros approved these changes Sep 13, 2024

View reviewed changes

ti-chi-bot bot merged commit 027f01b into pingcap:master Sep 13, 2024
33 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

br: migrate pitr id map to the system table `mysql.tidb_pitr_id_map` #55871

br: migrate pitr id map to the system table `mysql.tidb_pitr_id_map` #55871

Leavrth commented Sep 5, 2024 •

edited

Loading

tiprow bot commented Sep 5, 2024

codecov bot commented Sep 5, 2024 •

edited

Loading

3pointer Sep 9, 2024

Leavrth Sep 10, 2024

3pointer Sep 9, 2024

Leavrth Sep 10, 2024

Leavrth Sep 10, 2024

YuJuncen Sep 11, 2024

Leavrth Sep 11, 2024

ti-chi-bot bot commented Sep 12, 2024

yudongusa left a comment

lance6716 left a comment

Leavrth commented Sep 12, 2024

windtalker left a comment

Leavrth commented Sep 13, 2024

ti-chi-bot bot commented Sep 13, 2024

br: migrate pitr id map to the system table mysql.tidb_pitr_id_map #55871

br: migrate pitr id map to the system table mysql.tidb_pitr_id_map #55871

Conversation

Leavrth commented Sep 5, 2024 • edited Loading

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

tiprow bot commented Sep 5, 2024

codecov bot commented Sep 5, 2024 • edited Loading

Codecov Report

3pointer Sep 9, 2024

Choose a reason for hiding this comment

Leavrth Sep 10, 2024

Choose a reason for hiding this comment

3pointer Sep 9, 2024

Choose a reason for hiding this comment

Leavrth Sep 10, 2024

Choose a reason for hiding this comment

Leavrth Sep 10, 2024

Choose a reason for hiding this comment

YuJuncen Sep 11, 2024

Choose a reason for hiding this comment

Leavrth Sep 11, 2024

Choose a reason for hiding this comment

ti-chi-bot bot commented Sep 12, 2024

[LGTM Timeline notifier]

yudongusa left a comment

Choose a reason for hiding this comment

lance6716 left a comment

Choose a reason for hiding this comment

Leavrth commented Sep 12, 2024

windtalker left a comment

Choose a reason for hiding this comment

Leavrth commented Sep 13, 2024

ti-chi-bot bot commented Sep 13, 2024

br: migrate pitr id map to the system table `mysql.tidb_pitr_id_map` #55871

br: migrate pitr id map to the system table `mysql.tidb_pitr_id_map` #55871

Leavrth commented Sep 5, 2024 •

edited

Loading

codecov bot commented Sep 5, 2024 •

edited

Loading