lightning: support index KV checking of 'replace' mode for lightning post-import conflict detection #45926

lyzx2001 · 2023-08-09T02:52:42Z

What problem does this PR solve?

Issue Number: ref #45774

Problem Summary:

Currently lightning only supports remove mode for post-import conflict detection, but many customers request lightning to support replace mode for lightning post-import conflict detection.

We would like to support replace mode for lightning post-import conflict detection:
To resolve rows with conflict, instead of deleting all the rows that are engaged in conflict (the algorithm for remove), we delete some of the rows with conflict and reserve other rows that can be kept and not cause conflict anymore. Under this circumstance, we only delete the necessary rows to resolve conflicts, so that we can keep more original rows than remove mode as long as the conflicts are resolved.

This PR contains the algorithms for index KV checking. The algorithms for data KV checking will be submitted in the next PR.

What is changed and how it works?

Demo code for 'replace' mode of lightning post-import conflict detection:

https://github.com/lyzx2001/tidb-conflict-replace

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

…ction

Signed-off-by: lance6716 <lance6716@gmail.com>

lance6716

Seems ReplaceConflictKeys has different logic with our demo, please refine it.

br/pkg/lightning/backend/local/duplicate.go

br/pkg/lightning/errormanager/errormanager.go

Signed-off-by: lance6716 <lance6716@gmail.com>

…backend conflict detection

lyzx2001 · 2023-08-15T11:04:35Z

/cc @lance6716

lyzx2001 · 2023-08-30T07:00:22Z

/cc @D3Hunter

lyzx2001 · 2023-08-30T07:32:10Z

/retest

lyzx2001 · 2023-08-30T08:28:50Z

/retest

lyzx2001 · 2023-08-30T09:39:15Z

/retest

br/tests/lightning_duplicate_resolution_replace/config.toml

br/pkg/lightning/backend/local/duplicate.go

br/pkg/lightning/config/config.go

D3Hunter · 2023-08-30T14:52:58Z

br/pkg/lightning/errormanager/errormanager.go

+		if err := rawKeyRows.Err(); err != nil {
+			return errors.Trace(err)
+		}
+		if err := rawKeyRows.Close(); err != nil {


close it even on early return

br/pkg/lightning/errormanager/errormanager.go

D3Hunter · 2023-08-30T15:24:27Z

br/pkg/lightning/errormanager/errormanager.go

+				return errors.Trace(err)
+			}
+			// encode the row key of the data KV that needs to be deleted
+			rowKey := tablecodec.EncodeRowKeyWithHandle(tbl.Meta().ID, handle)


can we get rowkey using EncodeRowKey with rawHandle

I have tried to replace rowKey := tablecodec.EncodeRowKeyWithHandle(tbl.Meta().ID, handle) with rowKey := tablecodec.EncodeRowKey(tbl.Meta().ID, rawHandle), and it turns out the two returned rowKey are different:

rowKey from EncodeRowKeyWithHandle: 74800000000000004b5f728000000000000002 rowKey from EncodeRowKey: 74800000000000004b5f7274800000000000004b5f728000000000000002

If we use the result from tablecodec.EncodeRowKey, the unit test and integration test would fail, so we should just keep the current tablecodec.EncodeRowKeyWithHandle.

@lyzx2001 Seems rawHandle is rowKey

see #45926 (comment)

@lyzx2001 Seems rawHandle is rowKey

Thanks for reminding. The encoding for rowKey has been deleted, and I directly use rawHandle as the row key of the data KV that needs to be deleted.

You still use encoded rowKey in line 598?

You still use encoded rowKey in line 598?

In the newest commit I use rawHandle here

D3Hunter · 2023-08-30T15:44:30Z

br/pkg/lightning/errormanager/errormanager.go

+					logutil.Key("rawKey", rawKey),
+					zap.Binary("rawValue", rawValue))
+				// if rawKey equals to KV pair's key and rawValue equals to KV pair's value, this index KV needs to be deleted
+				if bytes.Equal(kvPair.Key, rawKey) && bytes.Equal(kvPair.Val, rawValue) {


we already got rowKey previously, in what case we have to decode into datum and encode again to check this row key DO generate the index key?

Here we first get the latest value of the index KV that has been covered, then find out all the KV pairs that are contained in the latest data KV. We need to make sure that the index KV in the latest data KV is indeed the index KV that has been covered, because if we have 3 rows with the same UK, the first time we detect the index KV has been covered, we delete its data KV is fine, but the second time we detect the index KV has been covered, now its data KV has already been deleted, we do not have to delete it again.
So if we can check that the index KV in the latest data KV is not equal to the index KV that has been covered, that means this data KV is unrelated to this index KV (maybe inserted by other rows), so here we cannot delete it, otherwise this would be a wrong deletion.

@D3Hunter an example is

(pk, uk)
(1, a)
(1, b)
(2, a)

(1, a) is overwriten by (2, a). We found a->1 is an overwriten index KV and we are considering if its data KV with key "1" can be deleted. We got the latest value of key "1" which is (1, b), and encode it to get all KV pairs which is [1->b, b->1]. Only if there is a->1 we dare to delete data KV with key "1"

@lyzx2001 can you put this part into comments before the check? helps to understand it.

@lyzx2001 can you put this part into comments before the check? helps to understand it.

updated

D3Hunter · 2023-08-31T10:52:29Z

br/pkg/lightning/errormanager/errormanager.go

+			}
+		}
+		if err := rawKeyRows.Err(); err != nil {
+			_ = rawKeyRows.Close()


there're other returns above, maybe uses a defer after get rawKeyRows

lance6716 · 2023-08-31T12:22:53Z

br/pkg/lightning/errormanager/errormanager.go

+					zap.Binary("value", kvPair.Val),
+					logutil.Key("rawKey", rawKey),
+					zap.Binary("rawValue", rawValue))
+				// if rawKey equals to KV pair's key and rawValue equals to KV pair's value, this index KV needs to be deleted


Suggested change

// if rawKey equals to KV pair's key and rawValue equals to KV pair's value, this index KV needs to be deleted

// if rawKey equals to KV pair's key and rawValue equals to KV pair's value, this data KV needs to be deleted

modified in previous commit

lyzx2001 · 2023-08-31T14:11:25Z

/retest

lyzx2001 · 2023-08-31T14:34:42Z

/retest

lyzx2001 · 2023-09-01T06:04:34Z

/retest

lyzx2001 · 2023-09-01T06:39:43Z

/retest

ti-chi-bot · 2023-09-01T07:17:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: D3Hunter, lance6716

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~~ [D3Hunter,lance6716]

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 · 2023-09-01T07:17:51Z

[LGTM Timeline notifier]

Timeline:

2023-08-30 03:44:59.313865621 +0000 UTC m=+1898663.862881593: ☑️ agreed by lance6716.
2023-09-01 07:17:50.690544943 +0000 UTC m=+2084235.239560930: ☑️ agreed by D3Hunter.

D3Hunter · 2023-09-01T07:18:42Z

/test pull-br-integration-test

lightning: support 'replace' mode for lightning backend conflict dete…

0b0106a

…ction

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 9, 2023

lance6716 changed the title ~~lightning: support 'replace' mode for lightning backend conflict detection~~ [WIP]lightning: support 'replace' mode for lightning backend conflict detection Aug 9, 2023

ti-chi-bot bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 9, 2023

lance6716 and others added 2 commits August 9, 2023 11:57

add UT

e2e8abb

Signed-off-by: lance6716 <lance6716@gmail.com>

Merge branch 'master' into issue45774-step2

24e8fea

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

lyzx2001 added 2 commits August 9, 2023 13:32

finish implementing checking index KV

d835f9b

revise checking index KV

43b744a

lyzx2001 changed the title ~~[WIP]lightning: support 'replace' mode for lightning backend conflict detection~~ [WIP]lightning: support index KV checking of 'replace' mode for lightning backend conflict detection Aug 9, 2023

add integration tests

1193c8e

ti-chi-bot bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 9, 2023

lyzx2001 added 3 commits August 9, 2023 18:06

update integration tests

69cb874

modify unit tests

7d4964a

modify unit tests

ee24d18

lance6716 reviewed Aug 10, 2023

View reviewed changes

lyzx2001 changed the title ~~[WIP]lightning: support index KV checking of 'replace' mode for lightning backend conflict detection~~ [WIP]lightning: support index KV checking of 'replace' mode for lightning post conflict detection Aug 14, 2023

lyzx2001 changed the title ~~[WIP]lightning: support index KV checking of 'replace' mode for lightning post conflict detection~~ [WIP]lightning: support index KV checking of 'replace' mode for lightning backend conflict detection Aug 14, 2023

lyzx2001 and others added 4 commits August 14, 2023 19:20

modify ReplaceConflictKeys

1cede0e

modify ReplaceConflictKeys

fceb5e5

add log

917a160

Signed-off-by: lance6716 <lance6716@gmail.com>

lightning: support index KV checking of 'replace' mode for lightning …

1211545

…backend conflict detection

lyzx2001 changed the title ~~[WIP]lightning: support index KV checking of 'replace' mode for lightning backend conflict detection~~ lightning: support index KV checking of 'replace' mode for lightning backend conflict detection Aug 15, 2023

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 Aug 15, 2023

lyzx2001 added 2 commits August 15, 2023 18:26

modify ResolveDuplicateRows

b8e5972

sort import for errormanager.go

3f64fcd

lyzx2001 changed the title ~~lightning: support index KV checking of 'replace' mode for lightning post conflict detection~~ lightning: support index KV checking of 'replace' mode for lightning post-import conflict detection Aug 30, 2023

ti-chi-bot bot requested a review from D3Hunter August 30, 2023 07:00

D3Hunter reviewed Aug 30, 2023

View reviewed changes

lyzx2001 added 2 commits August 31, 2023 17:32

update according to comment

bae3e26

update according to comment

6475ad4

lyzx2001 requested a review from D3Hunter August 31, 2023 09:51

update according to comment

0c1a22b

D3Hunter reviewed Aug 31, 2023

View reviewed changes

lance6716 reviewed Aug 31, 2023

View reviewed changes

update according to comment

44ae021

lyzx2001 requested a review from D3Hunter September 1, 2023 03:15

update comments

318e4dc

D3Hunter approved these changes Sep 1, 2023

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Sep 1, 2023

ti-chi-bot bot merged commit c9219e4 into pingcap:master Sep 1, 2023
15 of 22 checks passed

lyzx2001 mentioned this pull request Sep 8, 2023

lightning: support data KV checking of 'replace' mode for lightning post-import conflict detection #46763

Merged

12 tasks

lyzx2001 mentioned this pull request Oct 8, 2023

lightning: add complex integration tests for lightning post-import conflict detection "replace" mode #47460

Merged

13 tasks

lyzx2001 mentioned this pull request Nov 10, 2023

lightning: add complex unit tests for lightning post-import conflict detection "replace" mode #48514

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lightning: support index KV checking of 'replace' mode for lightning post-import conflict detection #45926

lightning: support index KV checking of 'replace' mode for lightning post-import conflict detection #45926

lyzx2001 commented Aug 9, 2023 •

edited

Loading

lance6716 left a comment

lyzx2001 commented Aug 15, 2023

lyzx2001 commented Aug 30, 2023

lyzx2001 commented Aug 30, 2023

lyzx2001 commented Aug 30, 2023

lyzx2001 commented Aug 30, 2023

D3Hunter Aug 30, 2023

D3Hunter Aug 30, 2023

lyzx2001 Aug 31, 2023

lance6716 Aug 31, 2023

lance6716 Aug 31, 2023

lyzx2001 Aug 31, 2023

lance6716 Aug 31, 2023

lyzx2001 Aug 31, 2023

D3Hunter Aug 30, 2023

lyzx2001 Aug 31, 2023 •

edited

Loading

lance6716 Aug 31, 2023

D3Hunter Sep 1, 2023

lyzx2001 Sep 1, 2023

D3Hunter Aug 31, 2023

lyzx2001 Aug 31, 2023

lance6716 Aug 31, 2023 •

edited

Loading

lyzx2001 Aug 31, 2023

lyzx2001 commented Aug 31, 2023

lyzx2001 commented Aug 31, 2023

lyzx2001 commented Sep 1, 2023

lyzx2001 commented Sep 1, 2023

ti-chi-bot bot commented Sep 1, 2023

ti-chi-bot bot commented Sep 1, 2023

D3Hunter commented Sep 1, 2023

	// if rawKey equals to KV pair's key and rawValue equals to KV pair's value, this index KV needs to be deleted
	// if rawKey equals to KV pair's key and rawValue equals to KV pair's value, this data KV needs to be deleted

lightning: support index KV checking of 'replace' mode for lightning post-import conflict detection #45926

lightning: support index KV checking of 'replace' mode for lightning post-import conflict detection #45926

Conversation

lyzx2001 commented Aug 9, 2023 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

lance6716 left a comment

Choose a reason for hiding this comment

lyzx2001 commented Aug 15, 2023

lyzx2001 commented Aug 30, 2023

lyzx2001 commented Aug 30, 2023

lyzx2001 commented Aug 30, 2023

lyzx2001 commented Aug 30, 2023

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

lyzx2001 Aug 31, 2023 • edited Loading

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

lance6716 Aug 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lyzx2001 commented Aug 31, 2023

lyzx2001 commented Aug 31, 2023

lyzx2001 commented Sep 1, 2023

lyzx2001 commented Sep 1, 2023

ti-chi-bot bot commented Sep 1, 2023

ti-chi-bot bot commented Sep 1, 2023

[LGTM Timeline notifier]

D3Hunter commented Sep 1, 2023

lyzx2001 commented Aug 9, 2023 •

edited

Loading

lyzx2001 Aug 31, 2023 •

edited

Loading

lance6716 Aug 31, 2023 •

edited

Loading