table/tables: remove int handle id from row to fix binlog issue (#30489)

[mjonss]

What problem does this PR solve?

The int handle id is still in the row when writing to binlog, which causes an error. We follow a similar path as for non-partitioned tables and removing the handle id from the row if it exists.

Issue Number: close #30489

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

• 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.

When updating a row which would move it from one partition to another and binlog is enabled, there was a column count check that failed, which is now corrected.```

[ti-chi-bot]

[REVIEW NOTIFICATION]

This pull request has not been approved.

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.

[sre-bot]

Code Coverage Details: https://codecov.io/github/pingcap/tidb/commit/b1c846857c8711e9d17f003a7369704e5c5c9f72

[tiancaiamao]

The root cause of this kind of bug is the vague coding protocol.

The signature of the table.Table interface is table.UpdateRecord(h kv.Handle, oldRow, newRow []types.Datum)
From the very beginning, the handle and table row are orthogonal, so there is nothing wrong about this method signature.

Later on, we have 'PK as handle', and later the 'comman handle', so the handle may be also included in the table row.

Things become complex from this time.
In some cases, we need the handle column, so we add an ExtraHandleColumn column to the end of the row.

So whether the oldRow newRow contain the handle column become vague.
The coding protocol for table.Table changes from its original design.

For example, 'insert into t values (...)' would call the AddRecord method, the is no handle column when AddRecord is called. It need to generate one.
'update t set xx = ...' may also call the AddRecord method (when the handle changes, delete the old one and add a new one)... this time maybe the caller goes with a handle column (Chunk reader fill the ExtraHandleColumn).
From different call stack, the protocol differs. Then AddRecord need to handle the difference for both cases.
The same dilemma for RemoveRecord, UpdateRecord, etc.

I think vague coding protocol is the root cause.
Hereby, a better (alternative) way is make the protocol clear: oldRow newRow contains just row data, no handle.
And then we can fix the caller instead the callee, we can remove that extra handle column in the updateExecutor to feed the correct data for table.RemoveRecord().

[tiancaiamao]

oldRow newRow contains just row data, no handle.

I mean no extra handle column we added manually.

[bb7133]

if !t.Meta().IsCommonHandle && !t.Meta().PKIsHandle && len(deleteData) > len(t.Cols()) {

IMHO it's tricky to remove the handle column in this way...Is there any existing code like this?

[mjonss]

I agree that it is tricky and I would not propose it if it would not been done like this here. And now when looking at the code in deleteSingleTableByChunk I think I agree with @tiancaiamao and will try to change the protocol/caller instead.

[ti-chi-bot]

@mjonss: PR needs rebase.

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.

[codecov]

Codecov Report

Merging #34109 (b1c8468) into master (ecb0c98) will decrease coverage by 0.0348%.
Report is 5206 commits behind head on master.
The diff coverage is 55.3047%.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #34109        +/-   ##
================================================
- Coverage   63.5589%   63.5242%   -0.0348%     
================================================
  Files           826        827         +1     
  Lines        269471     269738       +267     
================================================
+ Hits         171273     171349        +76     
- Misses        84636      84806       +170     
- Partials      13562      13583        +21     

[ti-chi-bot]

@mjonss: 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-mysql-client-test	b1c8468	link	true	/test pull-mysql-client-test
idc-jenkins-ci-tidb/check_dev_2	b1c8468	link	true	/test check-dev2
idc-jenkins-ci-tidb/unit-test	b1c8468	link	true	/test unit-test
idc-jenkins-ci-tidb/mysql-test	b1c8468	link	true	/test mysql-test
idc-jenkins-ci-tidb/build	b1c8468	link	true	/test build
pull-integration-ddl-test	b1c8468	link	true	/test pull-integration-ddl-test
idc-jenkins-ci-tidb/check_dev	b1c8468	link	true	/test check-dev
pull-br-integration-test	b1c8468	link	true	/test pull-br-integration-test
pull-lightning-integration-test	b1c8468	link	true	/test pull-lightning-integration-test
pull-integration-e2e-test	b1c8468	link	true	/test pull-integration-e2e-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.