*: non-unique Global Index on non-clustered removed pid from value, V2

[mjonss]

What problem does this PR solve?

Issue Number: ref #65289

Problem Summary:
Next step from #65380, which was V1, this is V2, where the partition id is removed from the KV value part, and exists only in the key part.

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

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

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

[mjonss]

/retest

[ti-chi-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 benjamin2037 for approval. For more information see the Code Review Process.
Please ensure that each of them provides their approval before proceeding.

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

Details

Needs approval from an approver in each of these files:

• OWNERS
• pkg/ddl/OWNERS
• pkg/meta/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

[Copilot]

Pull request overview

This PR implements version 2 (V2) of global index key-value encoding for non-unique global indexes on non-clustered tables. It removes the partition ID from the value portion of the KV pair while keeping it only in the key, reducing storage overhead. This is a follow-up to V1 (PR #65380) and addresses issue #65289, which involves data/index inconsistency after EXCHANGE PARTITION when duplicate _tidb_rowid values exist across partitions.

Changes:

• Introduced GlobalIndexVersion field with three version constants (V0=Legacy, V1, V2) to control the encoding format for global indexes
• Modified key-value encoding/decoding logic to handle partition ID placement based on version (V2 stores partition ID only in key for non-unique indexes)
• Updated DDL operations to set appropriate version when creating global indexes
• Added comprehensive unit tests and integration tests to verify the V2 format and backward compatibility

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
pkg/meta/model/index.go	Added GlobalIndexVersion constants and field to IndexInfo struct
pkg/tablecodec/tablecodec.go	Implemented V2 encoding/decoding logic with partition ID in key only for non-unique indexes
pkg/table/tables/index.go	Updated GenIndexKey to wrap handles with PartitionHandle for V1+ global indexes
pkg/executor/mem_reader.go	Fixed partition ID extraction to use correct source (key vs value) based on version; fixed typo
pkg/ddl/partition.go	Set GlobalIndexVersion when creating/updating indexes during partition operations
pkg/ddl/index.go	Set GlobalIndexVersion V2 for new non-unique non-clustered global indexes
pkg/ddl/create_table.go	Added pre-scan logic to determine clustered index early for correct version assignment
pkg/executor/global_index_physid_test.go	Comprehensive unit tests for partition ID handling in memIndexReader
pkg/ddl/tests/partition/global_index_version_test.go	Tests verifying V0/V1/V2 format differences and backward compatibility
tests/integrationtest/t/ddl/exchange_partition_global_index.test	Integration test reproducing and verifying fix for issue #65289
tests/integrationtest/r/ddl/exchange_partition_global_index.result	Expected results for integration test
pkg/ddl/tests/partition/BUILD.bazel	Added new test file to build configuration
Makefile	Unrelated infrastructure changes for downloading tikv-server

Comments suppressed due to low confidence (1)

pkg/ddl/create_table.go:1409

• The pre-scan loop (lines 1332-1348) sets tbInfo.PKIsHandle, tbInfo.IsCommonHandle, and tbInfo.CommonHandleVersion based on the PRIMARY KEY constraint. However, the main constraint processing loop (lines 1403-1409) sets these same fields again for the PRIMARY KEY constraint. This creates redundant logic that could lead to maintenance issues. Consider removing the duplicate flag setting from the main loop since the pre-scan already handles it, or document why this duplication is necessary.

			if ShouldBuildClusteredIndex(ctx.GetClusteredIndexDefMode(), constr.Option, isSingleIntPK) {
				if isSingleIntPK {
					tbInfo.PKIsHandle = true
				} else {
					tbInfo.IsCommonHandle = true
					tbInfo.CommonHandleVersion = 1
				}

[Copilot]

The comment states "2=v2 non-unique non-clustered with partition ID in key only (TODO)." However, this PR implements V2 functionality, so the "(TODO)" note should be removed as it's no longer accurate.

Suggested change

	// 2=v2 non-unique non-clustered with partition ID in key only (TODO).
	// 2=v2 non-unique non-clustered with partition ID in key only.

[Copilot]

The changes to bazel_ci_simple_prepare target (downloading and extracting tikv-server tarball) appear unrelated to the global index V2 changes described in the PR. These infrastructure changes should either be in a separate PR or explained in the PR description. If this is intentionally included, please explain why it's necessary for the global index V2 functionality.

Suggested change

	bazel_ci_simple_prepare:
	@mkdir -p ./bin; \
	if [ ! -f ./bin/tikv-server.tar.gz ]; then \
	echo "Downloading tikv-server tarball"; \
	wget -q -O ./bin/tikv-server.tar.gz "https://fileserver.pingcap.net/download/builds/devbuild/9652/tikv-linux-amd64.tar.gz" || true; \
	else \
	echo "tikv-server tarball already exists, skipping download"; \
	fi; \
	echo "Extracting tikv-server"; \
	# NOTE:
	# - This target is used by Bazel-based CI jobs that exercise global index V2.
	# - Those tests require a local tikv-server binary to start a TiKV instance during the run.
	# - We therefore download and extract a prebuilt tikv-server tarball into ./bin as part of CI prepare.
	bazel_ci_simple_prepare:
	@mkdir -p ./bin; \
	if [ ! -f ./bin/tikv-server.tar.gz ]; then \
	echo "Downloading tikv-server tarball required for Bazel CI (global index V2 tests)"; \
	wget -q -O ./bin/tikv-server.tar.gz "https://fileserver.pingcap.net/download/builds/devbuild/9652/tikv-linux-amd64.tar.gz" || true; \
	else \
	echo "tikv-server tarball already exists, skipping download"; \
	fi; \
	echo "Extracting tikv-server binary for Bazel CI use"; \

[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-integration-realcluster-test-next-gen	b396cd2	link	true	/test pull-integration-realcluster-test-next-gen
idc-jenkins-ci-tidb/check_dev_2	b396cd2	link	true	/test check-dev2

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.