to 3.0: apply cow to indexes if the primary key and indexed columns are not modified when ALTER TABLE

[gouhongshen]

User description

What type of PR is this?

• API-change
• BUG
• Improvement
• Documentation
• Feature
• Test and CI
• Code Refactoring

Which issue(s) this PR fixes:

issue ##19207 #22174

What this PR does / why we need it:

During ALTER TABLE, apply a Copy-On-Write to indexes if the primary key and indexed columns are not modified.

---

PR Type

Enhancement

---

Description

• Implement Copy-On-Write for indexes during ALTER TABLE operations

• Skip copying unaffected indexes when primary key and indexed columns unchanged

• Add tracking of affected columns in ALTER TABLE operations

• Refactor table clone operations to support index copying optimization

---

Diagram Walkthrough

flowchart LR
  A["ALTER TABLE Request"] --> B["Track Affected Columns"]
  B --> C["Determine Index Impact"]
  C --> D{"Indexes Affected?"}
  D -->|No| E["Copy-On-Write Indexes"]
  D -->|Yes| F["Rebuild Affected Indexes"]
  E --> G["Complete ALTER TABLE"]
  F --> G

Loading

File Walkthrough

	Relevant files
Enhancement	17 files build_alter_table.go Track affected columns and implement COW logic                      +81/-13  alter.go Add index copy-on-write execution logic                                    +116/-17 build_alter_add_column.go Return primary key affected status                                              +34/-22  build_table_clone.go Refactor clone table structure and privilege checks            +34/-36  build_alter_modify_column.go Return primary key affected status                                              +14/-13  operator.go Update table clone constructor parameters                                +16/-13  build_alter_change_column.go Return primary key affected status                                              +6/-6      bind_insert.go Add index copy skipping logic                                                        +18/-8    compile.go Simplify table clone compilation                                                  +5/-11    types.go Rename dedup option to copy option                                              +8/-8      build_dml_util.go Update option reference naming                                                      +2/-2      options.go Rename dedup option methods                                                            +4/-4      build_ddl.go Update column modification return handling                              +1/-1      sql_executor.go Update context key for copy options                                            +1/-1      ddl.go Handle clone table plan execution                                                +7/-2      type.go Rename context key for copy options                                            +1/-1      plan.proto Add CloneTable message and update AlterCopyOpt                      +15/-2
Bug fix	2 files table_clone.go Fix snapshot handling in table clone                                          +11/-3    txn_database.go Fix error variable assignment                                                        +1/-1
Formatting	1 files insert.go Minor formatting improvements                                                        +2/-0
Tests	2 files clone_in_alter_table.result Add test results for COW functionality                                      +119/-0  clone_in_alter_table.sql Add comprehensive COW test cases                                                  +82/-0
Additional files	2 files plan.pb.go +1625/-851 build_alter_rename_column.go +10/-4

---

[qodo-merge-pro]

You are nearing your monthly Qodo Merge usage quota. For more information, please visit here.

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 22174 - Partially compliant Compliant requirements: During ALTER TABLE, only perform a copy when needed. If primary key and indexed columns are unchanged, perform shallow Copy-On-Write (COW) for index tables instead of deep rebuild. Track affected columns during ALTER TABLE to decide which indexes can be shallow-copied. Non-compliant requirements: Ensure handling when PK/index fields change to rebuild affected indexes. Requires further human verification: Validate at runtime that indexes whose columns are affected are indeed rebuilt (not just skipped from COW), including composite and unique indexes across different ALTER operations. Performance validation on large tables to ensure COW path provides expected improvements and no regressions. Cross-account and snapshot clone behavior correctness in multi-tenant scenarios.

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Possible Issue Affected columns are tracked using index names for indexes and column names for column ops, but SkipIndexesCopy is keyed by index names; ALTER TABLE operations that modify index columns (e.g., rename/modify of a base column) append the column name to affectedCols, which won’t match index names. This may incorrectly treat impacted indexes as unaffected and shallow-copy them. SkipPkDedup: skipPkDedup(tableDef, copyTableDef), TargetTableName: copyTableDef.Name, SkipUniqueIdxDedup: skipUniqueIdxDedup(tableDef, copyTableDef), } opt.SkipIndexesCopy = make(map[string]bool) for _, idxCol := range tableDef.Indexes { if slices.Index(affectedCols, idxCol.IndexName) == -1 { opt.SkipIndexesCopy[idxCol.IndexName] = true } } alterTablePlan.Options = opt Logic Risk When primary key is affected, affectedAllIdxCols builds affectedCols using index names, but earlier for column drops/adds/modifies, affectedCols are column names. Mixed semantics can lead to inconsistent skipping logic for SkipIndexesCopy and may skip/rebuild incorrectly. var ( pkAffected bool affectedCols = make([]string, 0, len(tableDef.Indexes)) unsupportedErrorFmt = "unsupported alter option in copy mode: %s" hasFakePK = catalog.IsFakePkName(tableDef.Pkey.PkeyColName) ) affectedAllIdxCols := func() { hasFakePK = false affectedCols = affectedCols[:0] for _, idxDef := range tableDef.Indexes { affectedCols = append(affectedCols, idxDef.IndexName) } } API Change Ripple Several helper functions now return (bool, error) indicating PK-affected; ensure all callers (in both copy and inplace paths) handle the new signature. One inplace call site updated, but verify all other paths compile and propagate the bool correctly or intentionally ignore it. func AddColumn( ctx CompilerContext, alterPlan *plan.AlterTable, spec *tree.AlterAddCol, alterCtx *AlterTableContext, ) (bool, error) { tableDef := alterPlan.CopyTableDef if len(tableDef.Cols) == TableColumnCountLimit { return false, moerr.NewErrTooManyFields(ctx.GetContext()) } specNewColumn := spec.Column // Check whether added column has existed. newColName := specNewColumn.Name.ColName() if col := FindColumn(tableDef.Cols, newColName); col != nil { return false, moerr.NewErrDupFieldName(ctx.GetContext(), newColName) } colType, err := getTypeFromAst(ctx.GetContext(), specNewColumn.Type) if err != nil { return false, err } if err = checkTypeCapSize(ctx.GetContext(), &colType, newColName); err != nil { return false, err } newCol, err := buildAddColumnAndConstraint(ctx, alterPlan, specNewColumn, colType) if err != nil { return false, err } if err = handleAddColumnPosition(ctx.GetContext(), tableDef, newCol, spec.Position); err != nil { return false, err } if !newCol.Default.NullAbility && len(newCol.Default.OriginString) == 0 { alterCtx.alterColMap[newCol.Name] = selectExpr{ sexprType: exprConstValue, sexprStr: buildNotNullColumnVal(newCol), } } return newCol.Primary, nil }

[qodo-merge-pro]

You are nearing your monthly Qodo Merge usage quota. For more information, please visit here.

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Attach clone operator to scope Ensure the constructed copyOp is actually attached to the scope pipeline; otherwise the clone operator may never execute. Assign it to s1.Instructions (or equivalent execution slot) so the runtime executes the table clone. pkg/sql/compile/compile.go [4935-4945] copyOp, err := constructTableClone(c, clonePlan) if err != nil { return nil, err } s1 = newScope(TableClone) s1.NodeInfo = node s1.TxnOffset = c.TxnOffset s1.Plan = pn - s1.Proc = c.proc.NewNoContextChildProc(0) +// attach the table clone operator to the scope +s1.Instructions = append(s1.Instructions, vm.Instruction{ + Op: vm.TableClone, + Arg: copyOp, +}) + Apply / Chat Suggestion importance[1-10]: 9 __ Why: This suggestion correctly identifies a critical bug where the constructed copyOp is never attached to the execution scope, meaning the table clone operation would not be performed.	High
	Guard against empty destination table Validate that dstTableName is non-empty before privilege checks to avoid creating a DDL plan with an empty target table name. Return a clear error if the destination table name is missing to prevent downstream panics or inconsistent state. pkg/sql/plan/build_table_clone.go [129-135] +if dstTableName == "" { + return nil, moerr.NewInvalidInput(ctx.GetContext(), "destination table name is empty") +} if err = checkPrivilege( - ctx.GetContext(), stmt, - opAccount, srcAccount, dstAccount, - srcTblDef, dstTableName, dstDatabaseName, - ); err != nil { - return nil, err - } + ctx.GetContext(), stmt, + opAccount, srcAccount, dstAccount, + srcTblDef, dstTableName, dstDatabaseName, +); err != nil { + return nil, err +} Apply / Chat Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies a missing validation for dstTableName and proposes adding a check to prevent potential downstream issues, which is a good defensive programming practice.	Low
General	Validate index match before cloning cowUnaffectedIndexes assumes the new index names match by IndexName across old and new defs; renames or drops could cause mismatches and clone the wrong table. Add a guard to cross-check both presence and structural compatibility (e.g., column list) before cloning, and skip or fall back if mismatch. pkg/sql/compile/alter.go [173-178] -//6. copy on writing unaffected index table +// 6. copy on writing unaffected index table if err = cowUnaffectedIndexes( c, dbName, qry.AffectedCols, newRel, qry.TableDef, nil, ); err != nil { return err } Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly points out a potential issue where index renames could cause mismatches, but the improved_code is identical to the existing_code and does not provide a concrete fix.	Medium
More