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

[L-maple]

…l index

What problem does this PR solve?

Issue Number: close #49998, close #49995

Problem Summary:

• global index's pointGet / batch pointGet have not support clustered index.
• global index should have the right write(insert, insert ignore, insert ... on duplicate update, replace, delete, update) behavior.

What changed and how does it work?

Check List

Tests

• [x] 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

• [ ] Affects user behaviors
• [ ] Contains syntax changes
• [ ] Contains variable changes
• [ ] Contains experimental features
• [ ] Changes MySQL compatibility

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

[ti-chi-bot[bot]]

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.

[tiprow[bot]]

Hi @L-maple. 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.

[L-maple]

/assign @L-maple

[L-maple]

/cc @Defined2014 @mjonss

[L-maple]

replace and insert on duplicate update

---- Replied Message ---- | From | Mattias @.> | | Date | 01/26/2024 20:07 | | To | @.> | | Cc | Changjie @.>@.> | | Subject | Re: [pingcap/tidb] executor: fix point get under clustered table && insert err for global index (PR #50013) |

@mjonss commented on this pull request.

In pkg/executor/batch_checker.go:

@@ -48,6 +49,12 @@ type toBeCheckedRow struct { uniqueKeys []*keyValueWithDupInfo // t is the table or partition this row belongs to. t table.Table

• // oldRowTable only used for global index.
• // insert ... on duplicate key update inserts a row, but the global index uk may have existed.
• // the partition id in the global index value is the real tableID for old row.
• // use oldRowTable to temporarily store the table this old row belongs to.
• // TestGlobalIndexForClusteredIndexTable is a concrete case for the scenario.
• oldRowTable table.Table

Is it only used for REPLACE? or is it different from t for any other statement?

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

[Defined2014]

Hi @L-maple, #49998 already be fixed by https://github.com/pingcap/tidb/pull/52442. If you are still interested with this PR, you can modify it and keep the modifications for #49995.

If there is no response by next Monday (04/14), I will close this PR.

[L-maple]

Hi @L-maple, #49998 already be fixed by #52442. If you are still interested with this PR, you can modify it and keep the modifications for #49995.

If there is no response by next Monday (04/14), I will close this PR.

@Defined2014 Wonderful for #52442 ! I had considered exchange the sequence of AddRecord and RemoveRecord，didn't continue with it. OK, I will modify this PR for the #49995 .

[ti-chi-bot[bot]]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Once this PR has been reviewed and has the lgtm label, please assign tangenta for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

• OWNERS
• pkg/table/OWNERS
• pkg/tablecodec/OWNERS

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

[L-maple]

@Defined2014 Hi,big brother! I want to test the ut, but meet the following error on the master branch: liuchangjie@liuchangjie03 executor % go test -test.run TestGlobalIndexForInsertIgnoreOnDuplicate github.com/cloudfoundry/gosigar ../../../../../go/pkg/mod/github.com/cloudfoundry/[email protected]/concrete_sigar.go:20:12: cpuUsage.Get undefined (type Cpu has no field or method Get) ../../../../../go/pkg/mod/github.com/cloudfoundry/[email protected]/concrete_sigar.go:30:14: cpuUsage.Get undefined (type Cpu has no field or method Get) ../../../../../go/pkg/mod/github.com/cloudfoundry/[email protected]/concrete_sigar.go:49:11: l.Get undefined (type LoadAverage has no field or method Get) ../../../../../go/pkg/mod/github.com/cloudfoundry/[email protected]/concrete_sigar.go:55:11: m.Get undefined (type Mem has no field or method Get) ../../../../../go/pkg/mod/github.com/cloudfoundry/[email protected]/concrete_sigar.go:61:11: m.GetIgnoringCGroups undefined (type Mem has no field or method GetIgnoringCGroups) ../../../../../go/pkg/mod/github.com/cloudfoundry/[email protected]/concrete_sigar.go:67:11: s.Get undefined (type Swap has no field or method Get) ../../../../../go/pkg/mod/github.com/cloudfoundry/[email protected]/sigar_shared.go:12:21: procTime.Get undefined (type *ProcTime has no field or method Get) FAIL github.com/pingcap/tidb/pkg/executor [build failed] Is there any already known problem?

liuchangjie@liuchangjie03 executor % go env GO111MODULE='on' GOARCH='amd64' GOBIN='' GOCACHE='/Users/liuchangjie/Library/Caches/go-build' GOENV='/Users/liuchangjie/Library/Application Support/go/env' GOEXE='' GOEXPERIMENT='' GOFLAGS='' GOHOSTARCH='amd64' GOHOSTOS='darwin' GOINSECURE='' GOMODCACHE='/Users/liuchangjie/go/pkg/mod' GONOPROXY='*sankuai.com,dianpingoa.com' GONOSUMDB='*sankuai.com,dianpingoa.com' GOOS='darwin' GOPATH='/Users/liuchangjie/go' GOPRIVATE='*sankuai.com,dianpingoa.com' GOPROXY='https://goproxy.cn,direct' GOROOT='/usr/local/go1.22' GOSUMDB='off' GOTMPDIR='' GOTOOLCHAIN='' GOTOOLDIR='/usr/local/go1.22/pkg/tool/darwin_amd64' GOVCS='' GOVERSION='go1.22.2' GCCGO='gccgo' GOAMD64='v1' AR='ar' CC='clang' CXX='clang++' CGO_ENABLED='0' GOMOD='/Users/liuchangjie/go_projects/pingcap/tidb/go.mod' GOWORK='' CGO_CFLAGS='-O2 -g' CGO_CPPFLAGS='' CGO_CXXFLAGS='-O2 -g' CGO_FFLAGS='-O2 -g' CGO_LDFLAGS='-O2 -g' PKG_CONFIG='pkg-config' GOGCCFLAGS='-fPIC -arch x86_64 -m64 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/dp/d2my0q6x1fx9qcr_bp4s32tw0000gp/T/go-build376888664=/tmp/go-build -gno-record-gcc-switches -fno-common' liuchangjie@liuchangjie03 executor % go version go version go1.22.2 darwin/amd64

[Defined2014]

type Cpu has no field or method Ge

https://github.com/pingcap/tidb/issues/49742#issuecomment-1986823009 maybe you could follow this comment.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 83.33333% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 55.3882%. Comparing base (26e3b5f) to head (0487870). Report is 13 commits behind head on master.

Additional details and impacted files

@@                Coverage Diff                @@
##             master     #50013         +/-   ##
=================================================
- Coverage   72.3765%   55.3882%   -16.9883%     
=================================================
  Files          1482       1597        +115     
  Lines        428762     599564     +170802     
=================================================
+ Hits         310323     332088      +21765     
- Misses        99142     244394     +145252     
- Partials      19297      23082       +3785     

Flag	Coverage Δ
integration	22.9253% <44.4444%> (?)
unit	71.2285% <83.3333%> (+0.0163%)	:arrow_up:

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

Components	Coverage Δ
dumpling	53.9957% <ø> (ø)
parser	∅ <ø> (∅)
br	49.9665% <ø> (+8.7885%)	:arrow_up:

[L-maple]

@Defined2014 @mjonss Long time no chat, this PR can be reviewed again~

[Defined2014]

@Defined2014 @mjonss Long time no chat, this PR can be reviewed again~

Please fix failed test cases.

[L-maple]

@Defined2014 I test the check_dev on local, but it success. How should I pass the check_dev?

[L-maple]

/retest

[tiprow[bot]]

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

Test name	Commit	Details	Required	Rerun command
tiprow_fast_test	2940adb6a0d80a65171a4a65714df28bca637667	link	true	/test tiprow_fast_test
fast_test_tiprow	964fb4cc17b448bfa91c00c3003df06b57f64d72	link	true	/test fast_test_tiprow

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.

[L-maple]

@Defined2014 Hi, big brother! I am not very familiar with bazel. What does the following error mean? I run bazel run //:gazelle, but no new file generated on local mac... How should I solve this error? https://do.pingcap.net/jenkins/blue/organizations/jenkins/pingcap%2Ftidb%2Fghpr_build/detail/ghpr_build/8312/pipeline/

[Defined2014]

@Defined2014 I test the check_dev on local, but it success. How should I pass the check_dev?

Maybe you could merge master and run test again.

@Defined2014 Hi, big brother! I am not very familiar with bazel. What does the following error mean? I run bazel run //:gazelle, but no new file generated on local mac... How should I solve this error? https://do.pingcap.net/jenkins/blue/organizations/jenkins/pingcap%2Ftidb%2Fghpr_build/detail/ghpr_build/8312/pipeline/

I don't know that, you could ask @hawkingrei for help. But the problem is pkg/tablecodec/tablecodec.go:1748:26: errorf: should replace errors.New(fmt.Sprintf(...)) with fmt.Errorf(...) (all_revive).

[ti-chi-bot[bot]]

@L-maple: The following tests 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	2940adb6a0d80a65171a4a65714df28bca637667	link	true	/test pull-br-integration-test
pull-lightning-integration-test	2940adb6a0d80a65171a4a65714df28bca637667	link	true	/test pull-lightning-integration-test
idc-jenkins-ci-tidb/check_dev	048787044f08bbd459adabc8dbdc0c41d26b6d76	link	true	/test check-dev
idc-jenkins-ci-tidb/check_dev_2	048787044f08bbd459adabc8dbdc0c41d26b6d76	link	true	/test check-dev2

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]

Hi @L-maple, #49998 already be fixed by #52442. If you are still interested with this PR, you can modify it and keep the modifications for #49995.

If there is no response by next Monday (04/14), I will close this PR.

I opened a PR https://github.com/pingcap/tidb/pull/53254 which only contains the code related to issue #49995. If you have interests, you could review it.