*: fix `insert on duplicate` and `replace into` with global index #53717

Defined2014 · 2024-05-31T09:01:38Z

What problem does this PR solve?

Issue Number: close #53711, close #53750

Problem Summary:

When we deal with insert on duplicate, we always use the tableID of current value to found the old row. But the partition id between new and old values are different for global index.
Two functions DecodeHandleInUniqueIndexValue and DecodeHandleInIndexValue are similar, so I decided to remove one.

What changed and how does it work?

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-05-31T09:01:41Z

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-05-31T09:01:55Z

Hi @Defined2014. 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-05-31T09:24:28Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.6736%. Comparing base (b137425) to head (17ac9fe).
Report is 15 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #53717        +/-   ##
================================================
+ Coverage   72.5762%   74.6736%   +2.0973%     
================================================
  Files          1506       1506                
  Lines        430947     436505      +5558     
================================================
+ Hits         312765     325954     +13189     
+ Misses        98878      90531      -8347     
- Partials      19304      20020       +716

Flag	Coverage Δ
integration	`49.5308% <78.7234%> (?)`
unit	`71.4215% <93.6170%> (-0.1493%)`	⬇️

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

Components	Coverage Δ
dumpling	`52.9656% <ø> (-1.0302%)`	⬇️
parser	`∅ <ø> (∅)`
br	`50.3491% <ø> (+8.5147%)`	⬆️

zimulala · 2024-06-03T07:43:52Z

pkg/tablecodec/tablecodec.go

+	if len(value) <= MaxOldEncodeValueLen {
+		return decodeIntHandleInIndexValue(value), nil
+	}


Could we remove these codes that look like they will be processed on line988?

This speeds up the function for simplest int handles(like _tidb_rowid).

The performance difference is 9ns/op compare 15ns/op

mjonss

LGTM

mjonss · 2024-06-03T09:06:00Z

pkg/executor/admin.go

-		} else {
-			e.batchKeys = append(e.batchKeys, tablecodec.EncodeRecordKey(e.table.RecordPrefix(), h))
-		}
+		e.batchKeys = append(e.batchKeys, tablecodec.EncodeRecordKey(e.table.RecordPrefix(), h))


(Note for future reference)
Will this work with partitions? Is e.table the partition if non-global index, and the logical table if global index?
=> EncodeRecordKey now adds the proper PhysicalTableID (partition id) if partitioned handle.

Defined2014 · 2024-06-03T09:26:30Z

pkg/executor/insert_common.go

@@ -1373,7 +1373,11 @@ func (e *InsertValues) removeRow(
 		return true, nil
 	}

-	err = r.t.RemoveRecord(e.Ctx().GetTableCtx(), handle, oldRow)
+	if ph, ok := handle.(kv.PartitionHandle); ok {
+		err = e.Table.(table.PartitionedTable).GetPartition(ph.PartitionID).RemoveRecord(e.Ctx().GetTableCtx(), ph.Handle, oldRow)


Also Note for future reference.
We could fix it by handle partition handle in func (c *index) GenIndexKey, but since the phyTblID is already included in the index struct, so it is better to add it at the entry of removeRecord.

zimulala

LGTM

ti-chi-bot · 2024-06-03T11:29:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mjonss, zimulala

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~~ [mjonss,zimulala]
~~pkg/ddl/OWNERS~~ [zimulala]
~~pkg/table/OWNERS~~ [zimulala]
~~pkg/tablecodec/OWNERS~~ [zimulala]

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-06-03T11:29:42Z

[LGTM Timeline notifier]

Timeline:

2024-06-03 09:16:35.579888105 +0000 UTC m=+3286349.337023679: ☑️ agreed by mjonss.
2024-06-03 11:29:41.073933365 +0000 UTC m=+3294334.831068939: ☑️ agreed by zimulala.

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 31, 2024

Defined2014 added ok-to-test Indicates a PR is ready to be tested. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 31, 2024

Defined2014 marked this pull request as ready for review May 31, 2024 09:02

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 May 31, 2024

Defined2014 force-pushed the fix-53711 branch from c76e200 to 7cc5325 Compare May 31, 2024 09:20

ti-chi-bot bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 31, 2024

Defined2014 force-pushed the fix-53711 branch from 7cc5325 to 1800b53 Compare May 31, 2024 11:32

test

9d56715

Defined2014 force-pushed the fix-53711 branch from 1800b53 to 9d56715 Compare May 31, 2024 12:30

Defined2014 added 2 commits June 3, 2024 13:38

remove DecodeHandleInUniqueIndexValue func

2a6466e

add tests

4691829

ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 3, 2024

make fmt

ac19463

Defined2014 requested review from zimulala and mjonss June 3, 2024 06:26

zimulala reviewed Jun 3, 2024

View reviewed changes

Defined2014 added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 3, 2024

fix 53750

81afbd9

Defined2014 removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 3, 2024

ti-chi-bot bot added do-not-merge/needs-triage-completed and removed do-not-merge/needs-triage-completed labels Jun 3, 2024

Defined2014 changed the title *: fix insert on duplicate with global index *: fix insert on duplicate and replace into with global index Jun 3, 2024

update

17ac9fe

mjonss approved these changes Jun 3, 2024

View reviewed changes

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

Defined2014 commented Jun 3, 2024

View reviewed changes

zimulala approved these changes Jun 3, 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 Jun 3, 2024

ti-chi-bot bot merged commit f1ec74b into pingcap:master Jun 3, 2024
23 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: fix `insert on duplicate` and `replace into` with global index #53717

*: fix `insert on duplicate` and `replace into` with global index #53717

Defined2014 commented May 31, 2024 •

edited

Loading

ti-chi-bot bot commented May 31, 2024

tiprow bot commented May 31, 2024

codecov bot commented May 31, 2024 •

edited

Loading

zimulala Jun 3, 2024

Defined2014 Jun 3, 2024

Defined2014 Jun 3, 2024

zimulala Jun 3, 2024

mjonss left a comment

mjonss Jun 3, 2024

Defined2014 Jun 3, 2024

zimulala left a comment

ti-chi-bot bot commented Jun 3, 2024

ti-chi-bot bot commented Jun 3, 2024

*: fix insert on duplicate and replace into with global index #53717

*: fix insert on duplicate and replace into with global index #53717

Conversation

Defined2014 commented May 31, 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 May 31, 2024

tiprow bot commented May 31, 2024

codecov bot commented May 31, 2024 • edited Loading

Codecov Report

zimulala Jun 3, 2024

Choose a reason for hiding this comment

Defined2014 Jun 3, 2024

Choose a reason for hiding this comment

Defined2014 Jun 3, 2024

Choose a reason for hiding this comment

zimulala Jun 3, 2024

Choose a reason for hiding this comment

mjonss left a comment

Choose a reason for hiding this comment

mjonss Jun 3, 2024

Choose a reason for hiding this comment

Defined2014 Jun 3, 2024

Choose a reason for hiding this comment

zimulala left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Jun 3, 2024

ti-chi-bot bot commented Jun 3, 2024

[LGTM Timeline notifier]

*: fix `insert on duplicate` and `replace into` with global index #53717

*: fix `insert on duplicate` and `replace into` with global index #53717

Defined2014 commented May 31, 2024 •

edited

Loading

codecov bot commented May 31, 2024 •

edited

Loading