to hotfix: fix clone indexes when alter table. #23260

gouhongshen · 2025-12-11T06:02:24Z

User description

What type of PR is this?

Which issue(s) this PR fixes:

issue ##23258

What this PR does / why we need it:

Fixing the ALTER TABLE with a full-text index will cause index data duplication.

PR Type

Bug fix

Description

Prevent index data duplication during ALTER TABLE operations
Skip inserting into indexes marked for cloning in copy operations
Add skipIndexesCopy parameter to track indexes being cloned
Add comprehensive test cases for ALTER TABLE with various index types

Diagram Walkthrough

flowchart LR
  A["ALTER TABLE Operation"] --> B["Check skipIndexesCopy Map"]
  B --> C["Skip Insert for Cloned Indexes"]
  C --> D["Prevent Data Duplication"]
  E["AlterCopyOpt"] --> B

File Walkthrough

Relevant files

Bug fix

build_dml_util.go `Add index cloning skip logic to prevent duplication` pkg/sql/plan/build_dml_util.go Added `skipIndexesCoyp` variable to track indexes being cloned during ALTER TABLE Extract `SkipIndexesCopy` from `AlterCopyOpt` context when available Added logic to skip insert operations for indexes marked in `skipIndexesCopy` map Updated function signature of `buildInsertPlansWithRelatedHiddenTable` to accept `skipIndexesCopy` parameter Updated all callers to pass the new `skipIndexesCopy` parameter	+12/-3

Tests

clone_in_alter_table_2.sql `Add test cases for ALTER TABLE index cloning` test/distributed/cases/snapshot/clone/clone_in_alter_table_2.sql New test file with comprehensive test cases for ALTER TABLE with various index types Test case 1: FULLTEXT index with primary table and secondary key Test case 2: FULLTEXT index with multiple secondary keys Test case 3: Complex scenario with FULLTEXT, IVFFLAT, and HNSW vector indexes Validates index table row counts to ensure no data duplication occurs	+163/-0
clone_in_alter_table_2.result `Add expected test results for ALTER TABLE` test/distributed/cases/snapshot/clone/clone_in_alter_table_2.result Expected output results for the new test cases Validates correct row counts in index tables after ALTER TABLE operations Confirms no data duplication in FULLTEXT, secondary, IVFFLAT, and HNSW indexes	+149/-0

qodo-code-review · 2025-12-11T06:03:40Z

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance

⚪

Authorization/logic check

Description: The new skip-by-name check for indexes (skipIndexesCopy[indexdef.IndexName]) relies on
index names from context without explicit validation or namespace scoping, which could be
bypassed or misapplied if names collide or are user-controlled; verify that names are
trusted and map is constructed from internal metadata only.
build_dml_util.go [869-882]

Referred Code

multiTableIndexes := make(map[string]*MultiTableIndex)

for idx, indexdef := range tableDef.Indexes {
	if updateColLength == 0 {
		if indexdef.GetUnique() && (insertWithoutUniqueKeyMap != nil && insertWithoutUniqueKeyMap[indexdef.IndexName]) {
			continue
		}

		// we will clone this index data to new index table, skip insert.
		if skipIndexesCopy != nil && skipIndexesCopy[indexdef.IndexName] {
			continue
		}

		// append plan for the hidden tables of unique/secondary keys

Ticket Compliance

🟡

🎫 #23258

🟢	Fix bug where ALTER TABLE on a table with FULLTEXT index causes duplicated data in index/hidden index tables.
	Ensure ALTER TABLE column modification (e.g., MODIFY COLUMN default) does not duplicate rows in associated index tables.
	Provide tests demonstrating no duplication across FULLTEXT and other index types during ALTER TABLE.
🔴	Ensure fix applies to main and 3.0 branches context (hotfix branch is acceptable for 3.0.x).

Codebase Duplication Compliance

⚪

Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance

🟢

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

🔴

Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status:
Misspelled Name: The variable skipIndexesCoyp appears to be a misspelling of 'Copy', reducing
readability and clarity.

Referred Code

var skipIndexesCoyp map[string]bool

if v := builder.compCtx.GetContext().Value(defines.AlterCopyOpt{}); v != nil {
	dedupOpt := v.(*plan.AlterCopyOpt)
	if dedupOpt.TargetTableName == tableDef.Name {
		logutil.Info("alter copy dedup exec",
			zap.String("tableDef", tableDef.Name),
			zap.Bool("originalDedup", ifNeedCheckPkDup),
			zap.Any("insertWithoutUniqueKeyMap", insertWithoutUniqueKeyMap),
			zap.Any("dedupOpt", dedupOpt))
		if dedupOpt.SkipPkDedup {
			ifNeedCheckPkDup = false
		}
		if insertWithoutUniqueKeyMap == nil {
			insertWithoutUniqueKeyMap = dedupOpt.SkipUniqueIdxDedup
		} else {
			for skipIdxName := range dedupOpt.SkipUniqueIdxDedup {
				insertWithoutUniqueKeyMap[skipIdxName] = true
			}
		}
	}


 ... (clipped 7 lines)

Learn more about managing compliance generic rules or creating your own custom rules

⚪

Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
Lack Auditing: The new logic that skips index inserts based on skipIndexesCopy lacks any explicit audit
logging of this critical action in the shown changes, which may hinder reconstructing
events.

Referred Code

// we will clone this index data to new index table, skip insert.
if skipIndexesCopy != nil && skipIndexesCopy[indexdef.IndexName] {
	continue
}

// append plan for the hidden tables of unique/secondary keys
if indexdef.TableExist && catalog.IsRegularIndexAlgo(indexdef.IndexAlgo) {

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Nil Handling: The code relies on skipIndexesCopy and insertWithoutUniqueKeyMap without visible
validation or error paths in the new additions; while nil-checked, missing context on
conflicting flags or unexpected states may require broader handling not shown in diff.

Referred Code

multiTableIndexes := make(map[string]*MultiTableIndex)

for idx, indexdef := range tableDef.Indexes {
	if updateColLength == 0 {
		if indexdef.GetUnique() && (insertWithoutUniqueKeyMap != nil && insertWithoutUniqueKeyMap[indexdef.IndexName]) {
			continue
		}

		// we will clone this index data to new index table, skip insert.
		if skipIndexesCopy != nil && skipIndexesCopy[indexdef.IndexName] {
			continue
		}

		// append plan for the hidden tables of unique/secondary keys
		if indexdef.TableExist && catalog.IsRegularIndexAlgo(indexdef.IndexAlgo) {

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
Trusts Context: The code trusts AlterCopyOpt.SkipIndexesCopy from context without visible validation in
the diff, which may be acceptable if validated upstream but cannot be confirmed here.

Referred Code

if v := builder.compCtx.GetContext().Value(defines.AlterCopyOpt{}); v != nil {
	dedupOpt := v.(*plan.AlterCopyOpt)
	if dedupOpt.TargetTableName == tableDef.Name {
		logutil.Info("alter copy dedup exec",
			zap.String("tableDef", tableDef.Name),
			zap.Bool("originalDedup", ifNeedCheckPkDup),
			zap.Any("insertWithoutUniqueKeyMap", insertWithoutUniqueKeyMap),
			zap.Any("dedupOpt", dedupOpt))
		if dedupOpt.SkipPkDedup {
			ifNeedCheckPkDup = false
		}
		if insertWithoutUniqueKeyMap == nil {
			insertWithoutUniqueKeyMap = dedupOpt.SkipUniqueIdxDedup
		} else {
			for skipIdxName := range dedupOpt.SkipUniqueIdxDedup {
				insertWithoutUniqueKeyMap[skipIdxName] = true
			}
		}
	}
	skipIndexesCoyp = dedupOpt.SkipIndexesCopy
}


 ... (clipped 5 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

qodo-code-review · 2025-12-11T06:04:29Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category Suggestion Impact

General

Fix typo in variable name

Correct the typo in the variable name skipIndexesCoyp to skipIndexesCopy to fix
a compilation error and improve code consistency.

pkg/sql/plan/build_dml_util.go [160-188]

 	var fuzzymessage *OriginTableMessageForFuzzy
-	var skipIndexesCoyp map[string]bool
+	var skipIndexesCopy map[string]bool
 
 	if v := builder.compCtx.GetContext().Value(defines.AlterCopyOpt{}); v != nil {
 		dedupOpt := v.(*plan.AlterCopyOpt)
 		if dedupOpt.TargetTableName == tableDef.Name {
 			logutil.Info("alter copy dedup exec",
 				zap.String("tableDef", tableDef.Name),
 				zap.Bool("originalDedup", ifNeedCheckPkDup),
 				zap.Any("insertWithoutUniqueKeyMap", insertWithoutUniqueKeyMap),
 				zap.Any("dedupOpt", dedupOpt))
 			if dedupOpt.SkipPkDedup {
 				ifNeedCheckPkDup = false
 			}
 			if insertWithoutUniqueKeyMap == nil {
 				insertWithoutUniqueKeyMap = dedupOpt.SkipUniqueIdxDedup
 			} else {
 				for skipIdxName := range dedupOpt.SkipUniqueIdxDedup {
 					insertWithoutUniqueKeyMap[skipIdxName] = true
 				}
 			}
 		}
-		skipIndexesCoyp = dedupOpt.SkipIndexesCopy
+		skipIndexesCopy = dedupOpt.SkipIndexesCopy
 	}
 	return buildInsertPlansWithRelatedHiddenTable(stmt, ctx, builder, insertBindCtx, objRef, tableDef,
 		updateColLength, sourceStep, addAffectedRows, isFkRecursionCall, updatePkCol, pkFilterExpr,
 		newPartitionExpr, ifExistAutoPkCol, ifNeedCheckPkDup, indexSourceColTypes, fuzzymessage,
-		insertWithoutUniqueKeyMap, ifInsertFromUniqueColMap, nil, skipIndexesCoyp,
+		insertWithoutUniqueKeyMap, ifInsertFromUniqueColMap, nil, skipIndexesCopy,
 	)

Apply / Chat

Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a typo in the variable name skipIndexesCoyp. This is not just a style issue; it causes a compilation error because the function being called expects a parameter named skipIndexesCopy.

Medium

More

fix alter

87b7b29

gouhongshen marked this pull request as ready for review December 11, 2025 06:02

gouhongshen requested review from aunjgr, heni02 and ouyuanning as code owners December 11, 2025 06:02

qodo-code-review bot changed the title ~~to hotfix: fix clone indexes when alter table.~~ to hotfix: fix clone indexes when alter table. Dec 11, 2025

mergify bot requested a review from XuPeng-SH December 11, 2025 06:03

qodo-code-review bot added the Review effort 3/5 label Dec 11, 2025

mergify bot added the kind/bug Something isn't working label Dec 11, 2025

heni02 approved these changes Dec 11, 2025

View reviewed changes

heni02 merged commit 8b1b089 into matrixorigin:v3.0.4-hotfix Dec 11, 2025
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

to hotfix: fix clone indexes when alter table. #23260

to hotfix: fix clone indexes when alter table. #23260

Uh oh!

gouhongshen commented Dec 11, 2025 •

edited

Loading

Uh oh!

qodo-code-review bot commented Dec 11, 2025

Uh oh!

qodo-code-review bot commented Dec 11, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

to hotfix: fix clone indexes when alter table. #23260

to hotfix: fix clone indexes when alter table. #23260

Uh oh!

Conversation

gouhongshen commented Dec 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User description

What type of PR is this?

Which issue(s) this PR fixes:

What this PR does / why we need it:

PR Type

Description

Diagram Walkthrough

File Walkthrough

Uh oh!

qodo-code-review bot commented Dec 11, 2025

PR Compliance Guide 🔍

Uh oh!

qodo-code-review bot commented Dec 11, 2025

PR Code Suggestions ✨

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

gouhongshen commented Dec 11, 2025 •

edited

Loading