br: add table filter for log restore #57394

Tristan1900 · 2024-11-15T01:19:07Z

What problem does this PR solve?

Issue Number: close #57613

Problem Summary:
Need table filter for PiTR

What changed and how does it work?

The following happens if a custom filter is specified during PiTR.

Do a log backup meta scan at first before snapshot restore, and record all the table rename history and table mapping information during the scan, since we don't have access to genGlobalId, we use a temp id as a placeholder.
During snapshot restore, adjust the tables that need to be restored according to the rename history. It needs to add tables that renamed into the filter and removes the tables that are renamed out of the filter range during log backup. And put all the tables that need to be restored into a map/filter and pass down to log restore
During log restore, we use that map/filter from the above and filters out the tables/kvs that don't need to restore/rewrite, and we replace the temp id in the id map with real global ids.
For actual meta kv restore, use id map to filter out the kvs that doesn't need to restore. so for tables not exist in the id map, we can simply ignore it because they are filtered out.
The id map is persisted once built to work with checkpoint, so our filter naturally works with checkpoint as well.

Performance:
we are still scanning log meta kv twice, it's just we move the previously id map building step before snapshot happens.

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

ti-chi-bot · 2024-11-15T01:19:12Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

tiprow · 2024-11-15T01:19:25Z

Hi @Tristan1900. 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-11-25T01:03:05Z

Codecov Report

Attention: Patch coverage is 67.93570% with 379 lines in your changes missing coverage. Please review.

Project coverage is 74.2094%. Comparing base (c329b22) to head (aa216cd).

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #57394        +/-   ##
================================================
+ Coverage   73.0570%   74.2094%   +1.1524%     
================================================
  Files          1689       1709        +20     
  Lines        467012     467741       +729     
================================================
+ Hits         341185     347108      +5923     
+ Misses       104851      98852      -5999     
- Partials      20976      21781       +805

Flag	Coverage Δ
integration	`41.5676% <67.9357%> (?)`

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

Components	Coverage Δ
dumpling	`52.6910% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`61.1955% <67.9357%> (+15.7237%)`	⬆️

ti-chi-bot · 2025-01-01T23:59:08Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign leavrth for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

br/OWNERS

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

br/pkg/utils/schema.go

br/cmd/br/cmd.go

br/pkg/stream/rewrite_meta_rawkv.go

br/tests/config/tikv.toml

br/pkg/stream/table_mapping.go

Leavrth · 2025-01-03T06:05:45Z

br/pkg/stream/table_mapping.go

 	} else {
 		dr.Name = dbInfo.Name.O
 	}
 	return nil
 }

-func (tc *TableMappingManager) parseTableValueAndUpdateIdMapping(dbID int64, value []byte) error {
+func (tm *TableMappingManager) ProcessTableValueAndUpdateIdMapping(dbID int64, tableInfo model.TableInfo) error {


Why not pass by pointer, which is like tableInfo *model.TableInfo?

And remove tableInfo.ID = tableReplace.TableID and partitions.Definitions[i].ID = newID below.

good call, these are from the rewrite method, not needed here

br/pkg/stream/table_mapping.go

br/pkg/task/stream.go

br/pkg/restore/log_client/client.go

Leavrth · 2025-01-03T16:39:47Z

br/pkg/restore/log_client/batch_meta_processor.go

+			}
+
+			// collect table history indexed by table id, same id may have different table names in history
+			if meta.IsTableKey(rawKey.Field) {


else if meta.IsTableKey(rawKey.Field) {

Leavrth · 2025-01-03T16:47:09Z

br/pkg/stream/table_mapping.go

 	} else {
 		dr.Name = dbInfo.Name.O
 	}
 	return nil
 }

-func (tc *TableMappingManager) parseTableValueAndUpdateIdMapping(dbID int64, value []byte) error {
+func (tm *TableMappingManager) ProcessTableValueAndUpdateIdMapping(dbID int64, tableInfo model.TableInfo) error {


And remove tableInfo.ID = tableReplace.TableID and partitions.Definitions[i].ID = newID below.

Leavrth · 2025-01-03T17:06:00Z

br/pkg/task/restore.go

+			// check during log backup
+			dbName = name
+		} else {
+			log.Warn("did not find db id in full/log backup, "+


perhaps it's better to return an error because it must be a code-level error.

if we have full backup with --filter="foo*" and PiTR doesn't have any filter, it can have this problem, let me know your thoughts on how to better handle this situation

Leavrth

rest LGTM

Leavrth · 2025-01-06T03:19:55Z

br/pkg/task/restore.go

+	return
+}
+
+func adjustTablesToRestoreAndCreateTableTracker(


the function is important so it needs some unit tests.

Leavrth · 2025-01-06T03:41:36Z

br/pkg/stream/table_mapping.go

+		tempIDs = append(tempIDs, id)
+	}
+
+	// sort to -1, -2, -4, -8 ... etc


tempIDs[i] > tempIDs[j] does not means upstream(tempIDs[i]) > upstream[tempIDs[j]), so is it necessary to sort tempIDs?

yes it doesn't, I was thinking to assign id based on first seen first, somewhat like sorted by time in the log backup, I was hoping it can be a bit useful when debugging. The order doesn't suggest anything such as upstream id ordering.

Leavrth · 2025-01-06T03:46:05Z

br/pkg/task/restore.go

+			}
+
+			// need to restore the matching table in snapshot restore phase
+			for _, originalTable := range originalDB.Tables {


I prefer to merge filterRestoreFiles and adjustTablesToRestoreAndCreateTableTracker without any intermediates. That's because I am worried about the time complexity of the loop.

good question, discussed offline, the case of rename/exchange table should be rare so it should be fine, if it's problem we can convert it to be a map in future.

Tristan1900 · 2025-01-13T20:21:59Z

/retest

tiprow · 2025-01-13T20:22:27Z

@Tristan1900: 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.

YuJuncen · 2025-01-14T03:39:57Z

br/pkg/restore/log_client/batch_meta_processor.go

+		}
+
+		// write cf doesn't have short value in it
+		if value == nil {


Should we handle the DELETE operations in the write CF? (Which doesn't have a value.)

right, I think it's not necessarily needed, we are only tracking the all the table ids at this step, if there is a delete there must be a create table happened before, so that should be tracked already.

YuJuncen · 2025-01-14T03:57:09Z

br/pkg/restore/log_client/client.go

 	ctx context.Context,
 	s storage.ExternalStorage,
-	tableFilter filter.Filter,
+	piTRTableFilter *utils.PiTRTableTracker,


Perhaps name the variable piTRTableTracker or rename the PiTRTableTracker to something like TracedPiTRTableFilter? As tracker.ContainsDB(foo) -> Filtered Out looks a little bit strange.

agree, I want to avoid using filter here because I feel like the term is overloaded so can cause confusion. let me rename all to be tracker related. but actually tracker.ContainsDB(foo) -> included

YuJuncen · 2025-01-14T06:09:02Z

br/pkg/restore/snap_client/client.go

+func (rc *SnapClient) GetDatabaseMap() map[int64]*metautil.Database {
+	dbMap := make(map[int64]*metautil.Database)
+	for _, db := range rc.databases {
+		dbMap[db.Info.ID] = db
+	}
+	return dbMap


How about maps.Clone?

oh it's indexed differently if you mean to clone rc.databases. one is indexed by db name and this one is db id

YuJuncen · 2025-01-14T07:18:29Z

br/pkg/task/restore.go

@@ -1655,3 +1823,26 @@ func afterTableRestoredCh(ctx context.Context, createdTables []*snapclient.Creat
 	}()
 	return outCh
 }
+
+func convertMapsToSlices(


How about make a generic function like:

func convertMapToSlice[K, V any](m map[K]V) []V { values := make([]T, 0, len(m)) for v := range maps.Values(m) { values = append(values, v) } return values }

And call it three times?

YuJuncen · 2025-01-15T05:57:10Z

br/pkg/task/restore.go

+			}
+			// handle case where current is not in range and original was in range, we need to remove the original from
+			// restoring
+		} else if utils.MatchTable(cfg.TableFilter, dbName, start.TableName) {


Just wondering whether the table will be created again by putting meta again (By the meta entry of rename). I didn't found something that filters out not matched meta keys.

ok not sure about the meaning of this one, will follow up offline

YuJuncen · 2025-01-15T06:42:50Z

br/pkg/task/restore_test.go

+		},
+	}
+
+	// Test case 1: Basic table tracking


When there are many similar cases, would you parameterize those case? like:

type Case struct { Filter string PiTRHistories struct{ tid int64, tname string, did int64 } ExceptsTables [][2]int ExceptsDBNotExist []int ExcpetsTableNotExist []int }

Also you may warp each case with t.Run, so cases can be run independently.

YuJuncen · 2025-01-15T06:46:35Z

br/pkg/task/stream.go

 // gc.ratio-threshold = "-1.0", which represents disable gc in TiKV.
-func KeepGcDisabled(g glue.Glue, store kv.Storage) (RestoreFunc, string, error) {
+func DisableGc(g glue.Glue, store kv.Storage) (RestoreGcFunc, string, error) {


Go prefers captialize all letters in acronyms. As we are going to rename we may follow this tradition by the way.

Suggested change

func DisableGc(g glue.Glue, store kv.Storage) (RestoreGcFunc, string, error) {

func DisableGC(g glue.Glue, store kv.Storage) (RestoreGCFunc, string, error) {

got it, good to know!

Signed-off-by: Wenqi Mou <[email protected]>

ti-chi-bot · 2025-01-27T18:34:16Z

@Tristan1900: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
idc-jenkins-ci-tidb/unit-test	`aa216cd`	link	true	`/test unit-test`
idc-jenkins-ci-tidb/build	`aa216cd`	link	true	`/test build`
pull-br-integration-test	`aa216cd`	link	true	`/test pull-br-integration-test`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

Tristan1900 force-pushed the table-filter branch from 7c04493 to 8ef978b Compare November 22, 2024 01:21

ti-chi-bot bot removed do-not-merge/needs-tests-checked do-not-merge/needs-linked-issue labels Nov 22, 2024

Tristan1900 force-pushed the table-filter branch from 8ef978b to 02a2318 Compare November 25, 2024 00:04

Tristan1900 marked this pull request as ready for review November 25, 2024 00:06

ti-chi-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 25, 2024

Tristan1900 force-pushed the table-filter branch from 02a2318 to c29b578 Compare November 25, 2024 00:18

Tristan1900 force-pushed the table-filter branch 2 times, most recently from f624281 to 6ebc531 Compare November 25, 2024 15:53

Tristan1900 force-pushed the table-filter branch 6 times, most recently from 852dcff to b5132a9 Compare December 23, 2024 18:56

Tristan1900 force-pushed the table-filter branch 7 times, most recently from d2b68b8 to d72415c Compare December 30, 2024 23:30

Tristan1900 force-pushed the table-filter branch 3 times, most recently from 04d4ccf to f0eb49d Compare January 1, 2025 23:58

Tristan1900 force-pushed the table-filter branch 3 times, most recently from 2f06bd9 to 924a195 Compare January 2, 2025 01:32

Leavrth reviewed Jan 3, 2025

View reviewed changes

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

br/cmd/br/cmd.go Outdated Show resolved Hide resolved

br/pkg/stream/rewrite_meta_rawkv.go Outdated Show resolved Hide resolved

br/tests/config/tikv.toml Outdated Show resolved Hide resolved

Leavrth reviewed Jan 3, 2025

View reviewed changes

Tristan1900 force-pushed the table-filter branch from ee5d347 to 45bcdca Compare January 3, 2025 21:53

Leavrth reviewed Jan 6, 2025

View reviewed changes

Tristan1900 force-pushed the table-filter branch 2 times, most recently from 021494a to 1eee968 Compare January 13, 2025 17:37

Tristan1900 force-pushed the table-filter branch from 37f61af to 9b515fe Compare January 15, 2025 00:00

YuJuncen reviewed Jan 15, 2025

View reviewed changes

Tristan1900 force-pushed the table-filter branch 2 times, most recently from 0e3162c to 6940958 Compare January 22, 2025 18:40

rebase on master

aa216cd

Signed-off-by: Wenqi Mou <[email protected]>

Tristan1900 force-pushed the table-filter branch from 977507c to aa216cd Compare January 27, 2025 17:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

br: add table filter for log restore #57394

br: add table filter for log restore #57394

Tristan1900 commented Nov 15, 2024 •

edited

Loading

ti-chi-bot bot commented Nov 15, 2024

tiprow bot commented Nov 15, 2024

codecov bot commented Nov 25, 2024 •

edited

Loading

ti-chi-bot bot commented Jan 1, 2025

Leavrth Jan 3, 2025

Leavrth Jan 3, 2025

Tristan1900 Jan 7, 2025

Leavrth Jan 3, 2025

Leavrth Jan 3, 2025

Leavrth Jan 3, 2025

Tristan1900 Jan 7, 2025

Leavrth left a comment

Leavrth Jan 6, 2025

Leavrth Jan 6, 2025

Tristan1900 Jan 7, 2025

Leavrth Jan 6, 2025

Tristan1900 Jan 21, 2025

Tristan1900 commented Jan 13, 2025

tiprow bot commented Jan 13, 2025

YuJuncen Jan 14, 2025

Tristan1900 Jan 21, 2025

YuJuncen Jan 14, 2025

Tristan1900 Jan 21, 2025

YuJuncen Jan 14, 2025

Tristan1900 Jan 22, 2025

YuJuncen Jan 14, 2025

YuJuncen Jan 15, 2025

Tristan1900 Jan 22, 2025

YuJuncen Jan 15, 2025

Tristan1900 Jan 22, 2025

YuJuncen Jan 15, 2025

Tristan1900 Jan 22, 2025

ti-chi-bot bot commented Jan 27, 2025

	func DisableGc(g glue.Glue, store kv.Storage) (RestoreGcFunc, string, error) {
	func DisableGC(g glue.Glue, store kv.Storage) (RestoreGCFunc, string, error) {

br: add table filter for log restore #57394

Are you sure you want to change the base?

br: add table filter for log restore #57394

Conversation

Tristan1900 commented Nov 15, 2024 • edited Loading

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

ti-chi-bot bot commented Nov 15, 2024

tiprow bot commented Nov 15, 2024

codecov bot commented Nov 25, 2024 • edited Loading

Codecov Report

ti-chi-bot bot commented Jan 1, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Leavrth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tristan1900 commented Jan 13, 2025

tiprow bot commented Jan 13, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Jan 27, 2025

Tristan1900 commented Nov 15, 2024 •

edited

Loading

codecov bot commented Nov 25, 2024 •

edited

Loading