executor: fix update global index return duplicate key error #42311

L-maple · 2023-03-16T06:15:41Z

What problem does this PR solve?

Issue Number: close #42312

Problem Summary:

What is changed and how it works?

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

ti-chi-bot · 2023-03-16T06:15:43Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

Defined2014
mjonss

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

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

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

ti-chi-bot · 2023-03-16T06:15:51Z

Hi @L-maple. Thanks for your PR.

I'm waiting for a pingcap member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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/test-infra repository.

table/tables/index.go

errno/errcode.go

CbcWestwolf · 2023-03-17T02:59:36Z

To pass the check_dev, please run make errdoc and generate a new errors.toml

L-maple · 2023-03-17T06:18:25Z

To pass the check_dev, please run make errdoc and generate a new errors.toml

resolved

table/tables/partition.go

Defined2014

LGTM for this PR, Not sure should we concerned about rollback for this situation. /cc @tiancaiamao

tidb/table/tables/partition.go

Lines 1604 to 1609 in fc166ff

    
           // UpdateRecord should be side effect free, but there're two steps here. 
        
           // What would happen if step1 succeed but step2 meets error? It's hard 
        
           // to rollback. 
        
           // So this special order is chosen: add record first, errors such as 
        
           // 'Key Already Exists' will generally happen during step1, errors are 
        
           // unlikely to happen in step2.

mjonss

LGTM

mjonss · 2023-03-22T15:21:43Z

ddl/db_partition_test.go

@@ -1952,6 +1953,106 @@ func TestDropPartitionWithGlobalIndex(t *testing.T) {
 	require.Equal(t, 2, cnt)
 }

+func TestGlobalIndexInsertInDropPartition(t *testing.T) {


Is this test suppose to hit the change/bug or just there for completeness?

mjonss · 2023-03-22T15:22:07Z

ddl/db_partition_test.go

+	tk.MustQuery("select * from test_global use index(idx_b) order by a").Check(testkit.Rows("1 1 1", "2 11 11", "8 8 8", "13 12 12"))
+}
+
+func TestGlobalIndexUpdateInDropPartition(t *testing.T) {


Same here, is this test suppose to hit the change/bug or just there for completeness?

L-maple · 2023-03-22T15:32:46Z

just for complement. there is a ut for drop partition with global index，but no ut exist for write when ddl happens.

@@ -1952,6 +1953,106 @@ func TestDropPartitionWithGlobalIndex(t *testing.T) {

require.Equal(t, 2, cnt) } +func TestGlobalIndexInsertInDropPartition(t *testing.T) { Is this test suppose to hit the change/bug or just there for completeness? In ddl/db_partition_test.go:

+ tk.MustExec("drop table if exists test_global")

+ tk.MustExec(`create table test_global ( a int, b int, c int) + partition by range( a ) ( + partition p1 values less than (10), + partition p2 values less than (20), + partition p3 values less than (30) + );`) + tk.MustExec("alter table test_global add unique index idx_b (b);") + tk.MustExec("insert into test_global values (1, 1, 1), (8, 8, 8), (11, 11, 11), (12, 12, 12);") + tk.MustExec("update test_global set a = 2 where a = 11") + tk.MustExec("update test_global set a = 13 where a = 12") + tk.MustExec("analyze table test_global") + tk.MustQuery("select * from test_global use index(idx_b) order by a").Check(testkit.Rows("1 1 1", "2 11 11", "8 8 8", "13 12 12")) +} + +func TestGlobalIndexUpdateInDropPartition(t *testing.T) { Same here, is this test suppose to hit the change/bug or just there for completeness? — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: ***@***.***>

mjonss · 2023-03-22T16:06:05Z

table/tables/index.go

+		if c.idxInfo.Global {
+			if val, err := txn.GetMemBuffer().Get(context.Background(), key); err == nil {
+				segs := tablecodec.SplitIndexValue(val)
+				if len(segs.PartitionID) != 0 {
+					_, pid, err := codec.DecodeInt(segs.PartitionID)
+					if err != nil {
+						return err
+					}
+					if pid != c.phyTblID {
+						continue
+					}
+				}
+			}
+		}


I was a bit curious and I wonder if we should change the way we add the indexes here?
I mean if it is a global index, why add it with the physical table ids of the partitions?

tiancaiamao · 2023-03-24T12:02:26Z

/merge

ti-chi-bot · 2023-03-24T12:02:30Z

This pull request has been accepted and is ready to merge.

Commit hash: ed7b12b

mjonss · 2023-03-24T13:17:56Z

/retest

hawkingrei · 2023-03-25T07:03:57Z

/test all

hawkingrei · 2023-03-25T13:20:23Z

/retest

ti-chi-bot added do-not-merge/needs-linked-issue release-note-none size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-ok-to-test labels Mar 16, 2023

ti-chi-bot added do-not-merge/needs-triage-completed and removed do-not-merge/needs-linked-issue labels Mar 16, 2023

Defined2014 changed the title ~~fix: fix update global index and update reorg partition for global index~~ executor: fix update global index and update reorg partition for global index Mar 16, 2023

Defined2014 added ok-to-test and removed needs-ok-to-test labels Mar 16, 2023

Defined2014 reviewed Mar 16, 2023

View reviewed changes

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

ti-chi-bot removed the do-not-merge/needs-triage-completed label Mar 16, 2023

L-maple force-pushed the feature/write_for_global_index branch 2 times, most recently from 20def73 to 41af99a Compare March 16, 2023 12:35

CbcWestwolf reviewed Mar 17, 2023

View reviewed changes

errno/errcode.go Outdated Show resolved Hide resolved

L-maple force-pushed the feature/write_for_global_index branch from 41af99a to 59384c6 Compare March 17, 2023 02:38

L-maple force-pushed the feature/write_for_global_index branch from 59384c6 to 9dc4187 Compare March 17, 2023 06:16

L-maple force-pushed the feature/write_for_global_index branch 4 times, most recently from 71a6f7e to 28d808d Compare March 17, 2023 11:15

L-maple requested a review from Defined2014 March 17, 2023 11:15

L-maple changed the title ~~executor: fix update global index and update reorg partition for global index~~ executor: fix update global index return duplicate key error Mar 17, 2023

Defined2014 reviewed Mar 17, 2023

View reviewed changes

table/tables/partition.go Outdated Show resolved Hide resolved

fix: fix update global index and update reorg partition for global index

ed7b12b

L-maple force-pushed the feature/write_for_global_index branch from 28d808d to ed7b12b Compare March 20, 2023 03:52

L-maple requested a review from Defined2014 March 20, 2023 03:54

Defined2014 approved these changes Mar 20, 2023

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Mar 20, 2023

Defined2014 requested review from tiancaiamao and mjonss March 20, 2023 04:48

mjonss approved these changes Mar 22, 2023

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Mar 22, 2023

mjonss reviewed Mar 22, 2023

View reviewed changes

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 24, 2023

ti-chi-bot merged commit 9e353e5 into pingcap:master Mar 25, 2023

Defined2014 mentioned this pull request Apr 9, 2024

partition: reorganized the execution order of partition table update operators #52442

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

executor: fix update global index return duplicate key error #42311

executor: fix update global index return duplicate key error #42311

L-maple commented Mar 16, 2023 •

edited

ti-chi-bot commented Mar 16, 2023 •

edited

ti-chi-bot commented Mar 16, 2023

CbcWestwolf commented Mar 17, 2023

L-maple commented Mar 17, 2023

Defined2014 left a comment

mjonss left a comment

mjonss Mar 22, 2023

mjonss Mar 22, 2023

L-maple commented Mar 22, 2023 via email

mjonss Mar 22, 2023

tiancaiamao commented Mar 24, 2023

ti-chi-bot commented Mar 24, 2023

mjonss commented Mar 24, 2023

hawkingrei commented Mar 25, 2023

hawkingrei commented Mar 25, 2023

	// UpdateRecord should be side effect free, but there're two steps here.
	// What would happen if step1 succeed but step2 meets error? It's hard
	// to rollback.
	// So this special order is chosen: add record first, errors such as
	// 'Key Already Exists' will generally happen during step1, errors are
	// unlikely to happen in step2.

executor: fix update global index return duplicate key error #42311

executor: fix update global index return duplicate key error #42311

Conversation

L-maple commented Mar 16, 2023 • edited

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Mar 16, 2023 • edited

ti-chi-bot commented Mar 16, 2023

CbcWestwolf commented Mar 17, 2023

L-maple commented Mar 17, 2023

Defined2014 left a comment

Choose a reason for hiding this comment

mjonss left a comment

Choose a reason for hiding this comment

mjonss Mar 22, 2023

Choose a reason for hiding this comment

mjonss Mar 22, 2023

Choose a reason for hiding this comment

L-maple commented Mar 22, 2023 via email

mjonss Mar 22, 2023

Choose a reason for hiding this comment

tiancaiamao commented Mar 24, 2023

ti-chi-bot commented Mar 24, 2023

mjonss commented Mar 24, 2023

hawkingrei commented Mar 25, 2023

hawkingrei commented Mar 25, 2023

L-maple commented Mar 16, 2023 •

edited

ti-chi-bot commented Mar 16, 2023 •

edited