Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BR-restore should still perform checksum even when the schema checksum is missing #56373

Closed
kennytm opened this issue Sep 27, 2024 · 4 comments · Fixed by #56712
Closed

BR-restore should still perform checksum even when the schema checksum is missing #56373

kennytm opened this issue Sep 27, 2024 · 4 comments · Fixed by #56712
Labels
affects-6.5 This bug affects the 6.5.x(LTS) versions. affects-7.1 This bug affects the 7.1.x(LTS) versions. affects-7.5 This bug affects the 7.5.x(LTS) versions. affects-8.1 This bug affects the 8.1.x(LTS) versions. affects-8.5 This bug affects the 8.5.x(LTS) versions. component/br This issue is related to BR of TiDB. severity/moderate type/bug The issue is confirmed as a bug.

Comments

@kennytm
Copy link
Contributor

kennytm commented Sep 27, 2024

Bug Report

Please answer these questions before submitting your issue. Thanks!

1. Minimal reproduce step (Required)

  1. Perform br backup full --checksum=0
  2. Perform br restore full --checksum=1

2. What did you expect to see? (Required)

BR restore compare the ADMIN CHECKSUM with the sum of crc64xor of all files, which are always computed in br backup regardless of original --checksum.

3. What did you see instead (Required)

BR restore does not compare the checksum because it used the Schema's crc64xor which are not populated without --checksum=1.

4. What is your TiDB version? (Required)

v6.5 and above

@kennytm kennytm added type/bug The issue is confirmed as a bug. component/br This issue is related to BR of TiDB. affects-6.5 This bug affects the 6.5.x(LTS) versions. labels Sep 27, 2024
@kennytm
Copy link
Contributor Author

kennytm commented Sep 27, 2024

Split off from #43007 (comment).

Since that the table schema's crc64xor is just the sum of the table's data files, the schema checksum is redundant. BR restore could simply rely on the total of the file crc64xor. I suggest

  1. Create a br debug command to populate the table schemas' crc64xor fields, so the backupmeta can be used with br restore full --checksum=1 without upgrading BR (which has to match the target cluster version)
  2. Adjust BR restore to operate on the sum of file crc64xor, so --checksum=1 always works. If the schema crc64xor exists, do an additional check to ensure the two are equivalent.

@kennytm kennytm changed the title BR should still perform checksum even when the schema checksum is missing BR-restore should still perform checksum even when the schema checksum is missing Sep 27, 2024
@jebter jebter added severity/moderate affects-7.1 This bug affects the 7.1.x(LTS) versions. affects-7.5 This bug affects the 7.5.x(LTS) versions. affects-8.1 This bug affects the 8.1.x(LTS) versions. labels Oct 8, 2024
@BornChanger
Copy link
Contributor

/assign @Tristan1900

Copy link

ti-chi-bot bot commented Oct 15, 2024

@BornChanger: GitHub didn't allow me to assign the following users: Tristan1900.

Note that only pingcap members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @Tristan1900

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.

@Tristan1900
Copy link
Contributor

Tristan1900 commented Oct 15, 2024

After this fixing this issue I feel like we need to adjust the semantics of the --checksum flag and maybe hide it for backup, looks like when it enabled the only extra work it does is to compare the checksum of files from BR side to the original one in cop tikv, which is not recommended anymore per team's previous discussion.

@ti-chi-bot ti-chi-bot bot closed this as completed in 4f047be Nov 18, 2024
@BornChanger BornChanger added the affects-8.5 This bug affects the 8.5.x(LTS) versions. label Nov 19, 2024
Tristan1900 added a commit to Tristan1900/tidb that referenced this issue Dec 12, 2024
Tristan1900 added a commit to Tristan1900/tidb that referenced this issue Dec 12, 2024
Tristan1900 added a commit to Tristan1900/tidb that referenced this issue Dec 13, 2024
Tristan1900 added a commit to Tristan1900/tidb that referenced this issue Dec 13, 2024
Tristan1900 added a commit to Tristan1900/tidb that referenced this issue Dec 13, 2024
Tristan1900 added a commit to Tristan1900/tidb that referenced this issue Dec 13, 2024
Tristan1900 added a commit to Tristan1900/tidb that referenced this issue Dec 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects-6.5 This bug affects the 6.5.x(LTS) versions. affects-7.1 This bug affects the 7.1.x(LTS) versions. affects-7.5 This bug affects the 7.5.x(LTS) versions. affects-8.1 This bug affects the 8.1.x(LTS) versions. affects-8.5 This bug affects the 8.5.x(LTS) versions. component/br This issue is related to BR of TiDB. severity/moderate type/bug The issue is confirmed as a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants