table: fix wrong handle comparation in index double write #57418

joechenrh · 2024-11-15T09:23:46Z

What problem does this PR solve?

Issue Number: close #57414

Problem Summary:

What changed and how does it work?

Unlike other indices, global index encodes the value of its key-value pairs with a partition ID:

Lines 1630 to 1649 in 2ca497e

    
           func genIndexValueVersion0(loc *time.Location, tblInfo *model.TableInfo, idxInfo *model.IndexInfo, 
        
           	idxValNeedRestoredData bool, distinct bool, untouched bool, indexedValues []types.Datum, h kv.Handle, 
        
           	partitionID int64, buf []byte) ([]byte, error) { 
        
           	var idxVal []byte 
        
           	if buf == nil { 
        
           		idxVal = make([]byte, 0) 
        
           	} else { 
        
           		idxVal = buf[:0] 
        
           	} 
        
           	idxVal = append(idxVal, 0) 
        
           	newEncode := false 
        
           	tailLen := 0 
        
           	if !h.IsInt() && distinct { 
        
           		idxVal = encodeCommonHandle(idxVal, h) 
        
           		newEncode = true 
        
           	} 
        
           	if idxInfo.Global { 
        
           		idxVal = encodePartitionID(idxVal, partitionID) 
        
           		newEncode = true 
        
           	}

So during the double write process, we may do comparison between partition handles and non-partition handles.

tidb/pkg/table/tables/index.go

Lines 419 to 432 in 2ca497e

    
           if c.idxInfo.BackfillState != model.BackfillStateInapplicable { 
        
           	// #52914: the delete key is covered by the new ingested key, which shouldn't be deleted. 
        
           	originVal, err := getKeyInTxn(context.TODO(), txn, key) 
        
           	if err != nil { 
        
           		return err 
        
           	} 
        
           	if len(originVal) > 0 { 
        
           		oh, err := tablecodec.DecodeHandleInIndexValue(originVal) 
        
           		if err != nil { 
        
           			return err 
        
           		} 
        
           		if !h.Equal(oh) { 
        
           			okToDelete = false 
        
           		}

For example, in #57414, h is the handle of global index i1 whose type is PartitionHandle while oh is IntHandle of the current index. Thus, !h.Equal(oh) will return false.

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

tiprow · 2024-11-15T09:23:59Z

Hi @joechenrh. 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-15T09:59:39Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.2817%. Comparing base (2d9f829) to head (39b7b8b).
Report is 37 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #57418        +/-   ##
================================================
+ Coverage   72.9476%   75.2817%   +2.3341%     
================================================
  Files          1672       1717        +45     
  Lines        461826     477452     +15626     
================================================
+ Hits         336891     359434     +22543     
+ Misses       104197      95757      -8440     
- Partials      20738      22261      +1523

Flag	Coverage Δ
integration	`49.5796% <33.3333%> (?)`
unit	`72.4832% <100.0000%> (+0.1403%)`	⬆️

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

Components	Coverage Δ
dumpling	`52.7673% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`61.8957% <ø> (+16.7700%)`	⬆️

---- 🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests
JS Bundle Analysis - Avoid shipping oversized bundles

joechenrh · 2024-11-15T11:21:47Z

/ok-to-test

pkg/ddl/index_modify_test.go

pkg/ddl/index.go

pkg/table/tables/index.go

wjhuang2016 · 2024-11-18T07:41:18Z

pkg/table/tables/index.go

-						if !h.Equal(oh) {
+						// The handle passed in may be a `PartitionHandle`,
+						// so we can't directly do comparation with them.
+						if !handleEqual(h, oh) {


If the passed handle is PartaitionHandle, why oh isn't partitionHandle here?

Take the case in #57414 as example, the handle h is retrieved from FetchDuplicatedHandle. Since global index is first checked and duplication is found, we will get PartitionHandle.

tidb/pkg/executor/insert.go

Lines 272 to 273 in 043f833

for _, uk := range r.uniqueKeys {

_, handle, err := tables.FetchDuplicatedHandle(ctx, uk.newKey, true, txn, e.Table.Meta().ID)

But the newly added index is not global index, so oh is just IntHandle.

In addition, there may also be opposite situations that oh is PartitionHandle and h is IntHandle. But IntHandle can be compared with PartitionHandle correctly.

Defined2014 · 2024-11-18T07:51:21Z

pkg/table/tables/index.go

@@ -386,6 +386,13 @@ func needPresumeKeyNotExistsFlag(ctx context.Context, txn kv.Transaction, key, t
 	return false, nil
 }

+func handleEqual(dupHandle, prevHandle kv.Handle) bool {


Maybe we could change func (ph PartitionHandle) Equal(h Handle) bool function. Only both handles are partition handle, we need compare pid.

Maybe we could change func (ph PartitionHandle) Equal(h Handle) bool function.

I'm not sure if it will affect other code so I add a new function. Let me have a try.

joechenrh · 2024-11-18T08:31:35Z

pkg/ddl/index.go

-	if idxInfo.Global {
-		// 'handle' comes from reading directly from a partition, without partition id,
-		// so we can only compare the handle part of the key.
-		if ph, ok := h.(kv.PartitionHandle); ok && ph.Handle.Equal(handle) {
-			// table row has been back-filled already, OK to add the index entry
-			return nil
-		}
-	}


Just remove some unnecessary logic for comparation.

ti-chi-bot · 2024-11-19T01:11:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Defined2014, wjhuang2016

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~~ [Defined2014,wjhuang2016]
~~pkg/ddl/OWNERS~~ [wjhuang2016]
~~pkg/table/OWNERS~~ [wjhuang2016]

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-11-19T01:11:10Z

[LGTM Timeline notifier]

Timeline:

2024-11-18 06:22:13.185422736 +0000 UTC m=+855695.376291733: ☑️ agreed by Defined2014.
2024-11-19 01:11:09.378685834 +0000 UTC m=+923431.569554816: ☑️ agreed by wjhuang2016.

ti-chi-bot · 2024-11-19T02:45:23Z

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

…57478) close #57414

fix and add test

5d144fd

ti-chi-bot bot added do-not-merge/needs-triage-completed release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 15, 2024

Update

2d94b44

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

Update

b799c28

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

joechenrh added 2 commits November 18, 2024 10:39

Fix code

7459a2c

Rename test

d5a0ccf

joechenrh changed the title ~~table: fix wrong handle comparation in index merge~~ table: fix wrong handle comparation in index double write Nov 18, 2024

D3Hunter reviewed Nov 18, 2024

View reviewed changes

pkg/ddl/index_modify_test.go Outdated Show resolved Hide resolved

pkg/ddl/index.go Outdated Show resolved Hide resolved

Address comments

086f0d6

Defined2014 added the needs-cherry-pick-release-8.5 Should cherry pick this PR to release-8.5 branch. label Nov 18, 2024

Defined2014 approved these changes Nov 18, 2024

View reviewed changes

ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Nov 18, 2024

Defined2014 reviewed Nov 18, 2024

View reviewed changes

pkg/table/tables/index.go Outdated Show resolved Hide resolved

wjhuang2016 reviewed Nov 18, 2024

View reviewed changes

Defined2014 reviewed Nov 18, 2024

View reviewed changes

Move logic into Handle.Equal

39b7b8b

joechenrh commented Nov 18, 2024

View reviewed changes

wjhuang2016 approved these changes Nov 19, 2024

View reviewed changes

ti-chi-bot bot added approved lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Nov 19, 2024

ti-chi-bot bot merged commit 058d947 into pingcap:master Nov 19, 2024
27 checks passed

ti-chi-bot mentioned this pull request Nov 19, 2024

table: fix wrong handle comparation in index double write (#57418) #57478

Merged

13 tasks

ti-chi-bot bot pushed a commit that referenced this pull request Nov 22, 2024

table: fix wrong handle comparation in index double write (#57418) (#…

d2978bc

…57478) close #57414

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

table: fix wrong handle comparation in index double write #57418

table: fix wrong handle comparation in index double write #57418

joechenrh commented Nov 15, 2024 •

edited

Loading

tiprow bot commented Nov 15, 2024

codecov bot commented Nov 15, 2024 •

edited

Loading

joechenrh commented Nov 15, 2024

wjhuang2016 Nov 18, 2024

joechenrh Nov 18, 2024

Defined2014 Nov 18, 2024

joechenrh Nov 18, 2024

joechenrh Nov 18, 2024

ti-chi-bot bot commented Nov 19, 2024

ti-chi-bot bot commented Nov 19, 2024

ti-chi-bot commented Nov 19, 2024

	func genIndexValueVersion0(loc time.Location, tblInfo model.TableInfo, idxInfo *model.IndexInfo,
	idxValNeedRestoredData bool, distinct bool, untouched bool, indexedValues []types.Datum, h kv.Handle,
	partitionID int64, buf []byte) ([]byte, error) {
	var idxVal []byte
	if buf == nil {
	idxVal = make([]byte, 0)
	} else {
	idxVal = buf[:0]
	}
	idxVal = append(idxVal, 0)
	newEncode := false
	tailLen := 0
	if !h.IsInt() && distinct {
	idxVal = encodeCommonHandle(idxVal, h)
	newEncode = true
	}
	if idxInfo.Global {
	idxVal = encodePartitionID(idxVal, partitionID)
	newEncode = true
	}

	if c.idxInfo.BackfillState != model.BackfillStateInapplicable {
	// #52914: the delete key is covered by the new ingested key, which shouldn't be deleted.
	originVal, err := getKeyInTxn(context.TODO(), txn, key)
	if err != nil {
	return err
	}
	if len(originVal) > 0 {
	oh, err := tablecodec.DecodeHandleInIndexValue(originVal)
	if err != nil {
	return err
	}
	if !h.Equal(oh) {
	okToDelete = false
	}

	for _, uk := range r.uniqueKeys {
	_, handle, err := tables.FetchDuplicatedHandle(ctx, uk.newKey, true, txn, e.Table.Meta().ID)

table: fix wrong handle comparation in index double write #57418

table: fix wrong handle comparation in index double write #57418

Conversation

joechenrh commented Nov 15, 2024 • edited Loading

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

tiprow bot commented Nov 15, 2024

codecov bot commented Nov 15, 2024 • edited Loading

Codecov Report

joechenrh commented Nov 15, 2024

wjhuang2016 Nov 18, 2024

Choose a reason for hiding this comment

joechenrh Nov 18, 2024

Choose a reason for hiding this comment

Defined2014 Nov 18, 2024

Choose a reason for hiding this comment

joechenrh Nov 18, 2024

Choose a reason for hiding this comment

joechenrh Nov 18, 2024

Choose a reason for hiding this comment

ti-chi-bot bot commented Nov 19, 2024

ti-chi-bot bot commented Nov 19, 2024

[LGTM Timeline notifier]

ti-chi-bot commented Nov 19, 2024

joechenrh commented Nov 15, 2024 •

edited

Loading

codecov bot commented Nov 15, 2024 •

edited

Loading