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

Defined2014 · 2024-04-09T06:40:49Z

What problem does this PR solve?

Issue Number: close #52380, close #49998

Problem Summary: As updateRecord, we should remove record first and use memBuffer.Staging() to restore it when we meet errors in add records.

Ref code link

tidb/pkg/executor/write.go

Lines 165 to 195 in 510f44f

    
           if handleChanged { 
        
           	// For `UPDATE IGNORE`/`INSERT IGNORE ON DUPLICATE KEY UPDATE` 
        
           	// we use the staging buffer so that we don't need to precheck the existence of handle or unique keys by sending 
        
           	// extra kv requests, and the remove action will not take effect if there are conflicts. 
        
           	if updated, err := func() (bool, error) { 
        
           		txn, err := sctx.Txn(true) 
        
           		if err != nil { 
        
           			return false, err 
        
           		} 
        
           		memBuffer := txn.GetMemBuffer() 
        
           		sh := memBuffer.Staging() 
        
           		defer memBuffer.Cleanup(sh) 
        
           		if err = t.RemoveRecord(sctx.GetTableCtx(), h, oldData); err != nil { 
        
           			return false, err 
        
           		} 
        
           		_, err = t.AddRecord(sctx.GetTableCtx(), newData, table.IsUpdate, table.WithCtx(ctx)) 
        
           		if err != nil { 
        
           			return false, err 
        
           		} 
        
           		memBuffer.Release(sh) 
        
           		return true, nil 
        
           	}(); err != nil { 
        
           		if terr, ok := errors.Cause(err).(*terror.Error); ok && (terr.Code() == errno.ErrNoPartitionForGivenValue || terr.Code() == errno.ErrRowDoesNotMatchGivenPartitionSet) { 
        
           			ec := sctx.GetSessionVars().StmtCtx.ErrCtx() 
        
           			return false, ec.HandleError(err) 
        
           		} 
        
           		return updated, err 
        
           	} 
        
           } else {

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

tiprow · 2024-04-09T06:41:18Z

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

Defined2014 · 2024-04-09T06:41:58Z

pkg/table/tables/index.go

@@ -355,15 +354,6 @@ func (c *index) Create(sctx table.MutateContext, txn kv.Transaction, indexedValu
 			}
 			continue
 		}
-		if c.idxInfo.Global && len(value) != 0 && !bytes.Equal(value, idxVal) {


Added by #42311. After change the update operators, no need this code anymore.

Defined2014 · 2024-04-09T07:24:19Z

/retest

tiprow · 2024-04-09T07:24:40Z

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

Defined2014 · 2024-04-09T08:08:06Z

pkg/table/tables/partition.go

+				memBuffer.Cleanup(sh)
+			}
+		}()
+
 		err = t.GetPartition(from).RemoveRecord(ctx, h, currData)


As updateRecord, remove record first.

pkg/table/tables/partition.go

mjonss · 2024-04-09T21:46:31Z

@Defined2014 I assume this will also close #49998 ?

codecov · 2024-04-10T01:38:37Z

Codecov Report

Merging #52442 (152a13b) into master (035f5a3) will increase coverage by 2.5465%.
Report is 30 commits behind head on master.
The diff coverage is 73.6842%.

❗ Current head 152a13b differs from pull request most recent head b886ec1. Consider uploading reports for the commit b886ec1 to get more accurate results

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #52442        +/-   ##
================================================
+ Coverage   72.2286%   74.7752%   +2.5465%     
================================================
  Files          1467       1467                
  Lines        426789     431049      +4260     
================================================
+ Hits         308264     322318     +14054     
+ Misses        99427      88735     -10692     
- Partials      19098      19996       +898

Flag	Coverage Δ
integration	`49.2246% <73.6842%> (?)`
unit	`71.0151% <42.1052%> (-0.1027%)`	⬇️

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

Components	Coverage Δ
dumpling	`53.9957% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`53.2190% <ø> (+12.3195%)`	⬆️

ti-chi-bot · 2024-04-10T02:31:15Z

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

Test name	Commit	Details	Required	Rerun command
pull-br-integration-test	`152a13b`	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/test-infra repository. I understand the commands that are listed here.

Defined2014 · 2024-04-10T02:35:25Z

/retest

tiprow · 2024-04-10T02:35:48Z

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

zimulala

LGTM

Defined2014 · 2024-04-10T02:53:46Z

@Defined2014 I assume this will also close #49998 ?

Yes, linked.

tiancaiamao · 2024-04-10T06:41:38Z

pkg/table/tables/partition.go

 	// The old and new data locate in different partitions.
 	// Remove record from old partition and add record to new partition.
 	if from != to {
-		_, err = t.GetPartition(to).AddRecord(ctx, newData)


Why the old code does not get duplicate key error in this AddRecord operation?

Because the deleted code https://github.com/pingcap/tidb/pull/52442/files#r1557015648

ti-chi-bot · 2024-04-10T06:46:20Z

[LGTM Timeline notifier]

Timeline:

2024-04-10 02:50:16.458669975 +0000 UTC m=+1017077.986210521: ☑️ agreed by zimulala.
2024-04-10 06:46:19.949320696 +0000 UTC m=+1031241.476861244: ☑️ agreed by tiancaiamao.

mjonss

LGTM

ti-chi-bot · 2024-04-10T07:30:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mjonss, tiancaiamao, 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,tiancaiamao,zimulala]
~~pkg/table/OWNERS~~ [zimulala]

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

…operators (pingcap#52442) close pingcap#49998, close pingcap#52380

update

152a13b

ti-chi-bot bot added release-note-none size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 9, 2024

Defined2014 commented Apr 9, 2024

View reviewed changes

Defined2014 requested review from tiancaiamao, zimulala and mjonss April 9, 2024 07:56

Defined2014 commented Apr 9, 2024

View reviewed changes

mjonss reviewed Apr 9, 2024

View reviewed changes

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

update

b648100

Defined2014 force-pushed the fix-52380 branch from b886ec1 to b648100 Compare April 10, 2024 01:39

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

Defined2014 added 2 commits April 10, 2024 10:07

remove record first

f59dccb

update

0ea374e

zimulala approved these changes Apr 10, 2024

View reviewed changes

ti-chi-bot bot added approved needs-1-more-lgtm labels Apr 10, 2024

tiancaiamao reviewed Apr 10, 2024

View reviewed changes

tiancaiamao approved these changes Apr 10, 2024

View reviewed changes

ti-chi-bot bot added the lgtm label Apr 10, 2024

ti-chi-bot bot removed the needs-1-more-lgtm label Apr 10, 2024

mjonss approved these changes Apr 10, 2024

View reviewed changes

ti-chi-bot bot merged commit e540f0f into pingcap:master Apr 10, 2024
23 of 24 checks passed

Defined2014 deleted the fix-52380 branch April 10, 2024 07:37

Defined2014 mentioned this pull request Apr 10, 2024

executor: fix point get under clustered table && insert err for global index #50013

Open

13 tasks

3AceShowHand pushed a commit to 3AceShowHand/tidb that referenced this pull request Apr 16, 2024

partition: reorganized the execution order of partition table update …

7f63a85

…operators (pingcap#52442) close pingcap#49998, close pingcap#52380

L-maple mentioned this pull request May 15, 2024

table, executor: let SplitIndexValue work with clustered index table. #53254

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Defined2014 commented Apr 9, 2024 •

edited

tiprow bot commented Apr 9, 2024

Defined2014 Apr 9, 2024

Defined2014 commented Apr 9, 2024

tiprow bot commented Apr 9, 2024

Defined2014 Apr 9, 2024

mjonss commented Apr 9, 2024

codecov bot commented Apr 10, 2024

ti-chi-bot bot commented Apr 10, 2024 •

edited

Defined2014 commented Apr 10, 2024

tiprow bot commented Apr 10, 2024

zimulala left a comment

Defined2014 commented Apr 10, 2024

tiancaiamao Apr 10, 2024

Defined2014 Apr 10, 2024

ti-chi-bot bot commented Apr 10, 2024

mjonss left a comment

ti-chi-bot bot commented Apr 10, 2024

	if handleChanged {
	// For `UPDATE IGNORE`/`INSERT IGNORE ON DUPLICATE KEY UPDATE`
	// we use the staging buffer so that we don't need to precheck the existence of handle or unique keys by sending
	// extra kv requests, and the remove action will not take effect if there are conflicts.
	if updated, err := func() (bool, error) {
	txn, err := sctx.Txn(true)
	if err != nil {
	return false, err
	}
	memBuffer := txn.GetMemBuffer()
	sh := memBuffer.Staging()
	defer memBuffer.Cleanup(sh)

	if err = t.RemoveRecord(sctx.GetTableCtx(), h, oldData); err != nil {
	return false, err
	}

	_, err = t.AddRecord(sctx.GetTableCtx(), newData, table.IsUpdate, table.WithCtx(ctx))
	if err != nil {
	return false, err
	}
	memBuffer.Release(sh)
	return true, nil
	}(); err != nil {
	if terr, ok := errors.Cause(err).(*terror.Error); ok && (terr.Code() == errno.ErrNoPartitionForGivenValue \|\| terr.Code() == errno.ErrRowDoesNotMatchGivenPartitionSet) {
	ec := sctx.GetSessionVars().StmtCtx.ErrCtx()
	return false, ec.HandleError(err)
	}
	return updated, err
	}
	} else {

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

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

Conversation

Defined2014 commented Apr 9, 2024 • edited

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

tiprow bot commented Apr 9, 2024

Defined2014 Apr 9, 2024

Choose a reason for hiding this comment

Defined2014 commented Apr 9, 2024

tiprow bot commented Apr 9, 2024

Defined2014 Apr 9, 2024

Choose a reason for hiding this comment

mjonss commented Apr 9, 2024

codecov bot commented Apr 10, 2024

Codecov Report

ti-chi-bot bot commented Apr 10, 2024 • edited

Defined2014 commented Apr 10, 2024

tiprow bot commented Apr 10, 2024

zimulala left a comment

Choose a reason for hiding this comment

Defined2014 commented Apr 10, 2024

tiancaiamao Apr 10, 2024

Choose a reason for hiding this comment

Defined2014 Apr 10, 2024

Choose a reason for hiding this comment

ti-chi-bot bot commented Apr 10, 2024

[LGTM Timeline notifier]

mjonss left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Apr 10, 2024

Defined2014 commented Apr 9, 2024 •

edited

ti-chi-bot bot commented Apr 10, 2024 •

edited