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 functionality to support index COW 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["Full Index Rebuild"]
  E --> G["Clone Unaffected Index Tables"]
  F --> G
  G --> H["Complete ALTER Operation"]

Loading

File Walkthrough

	Relevant files
Enhancement	17 files build_alter_table.go Track affected columns and implement COW logic                      +81/-13  alter.go Add COW index copying functionality                                            +114/-17 build_table_clone.go Refactor clone table structure for COW support                      +67/-33  build_alter_add_column.go Return primary key affected status                                              +34/-22  build_alter_modify_column.go Return primary key affected status                                              +14/-13  build_alter_rename_column.go Return primary key affected status                                              +10/-4    build_alter_change_column.go Return primary key affected status                                              +6/-6      operator.go Update table clone constructor parameters                                +16/-13  bind_insert.go Add skip indexes copy option handling                                        +18/-8    compile.go Update table clone compilation flow                                            +11/-12  types.go Rename dedup option to copy option                                              +8/-8      options.go Update option method names                                                              +4/-4      build_dml_util.go Update context key reference                                                          +2/-2      sql_executor.go Update context key reference                                                          +1/-1      ddl.go Update table clone execution flow                                                +7/-2      type.go Rename context key type                                                                    +1/-1      plan.proto Add CloneTable message and update AlterCopyOpt                      +15/-2
Bug fix	1 files table_clone.go Fix snapshot handling in table clone                                          +11/-3
Tests	2 files clone_in_alter_table.sql Add comprehensive test cases for COW functionality              +77/-0    clone_in_alter_table.result Expected test results for COW functionality                            +102/-0
Additional files	5 files error.go +5/-0      plan.pb.go +1625/-851 insert.go +2/-0      build_ddl.go +1/-1      txn_database.go +1/-1

---

[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: Apply shallow copy / copy-on-write (COW) for index tables if there is no change to primary key and indexed columns. Only perform full copy or rebuild when a copy is needed or when PK/index fields change. Non-compliant requirements: During ALTER TABLE, avoid deep copying index tables when possible. Requires further human verification: Validate through integration tests and performance measurements that unaffected index tables are not deeply copied and that query plans use the cloned index tables. Verify correctness across different ALTER TABLE variants and mixed workloads (concurrency, tombstones, publication/tenant scenarios).

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Possible Issue The tracking of affected columns mixes index names and column names (affectedCols contains index names via idxDef.IndexName and also column names from ALTER specs). Later, SkipIndexesCopy checks membership by index name. This mismatch can cause unintended shallow copies or missed COW when a column rename/type change affects an index. 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) } } for _, spec := range validAlterSpecs { switch option := spec.(type) { case *tree.AlterOptionAdd: switch optionAdd := option.Def.(type) { case *tree.PrimaryKeyIndex: err = AddPrimaryKey(cctx, alterTablePlan, optionAdd, alterTableCtx) affectedAllIdxCols() default: // column adding is handled in *tree.AlterAddCol // various indexes\fks adding are handled in inplace mode. return nil, moerr.NewInvalidInputf(ctx, unsupportedErrorFmt, formatTreeNode(option)) } case *tree.AlterOptionDrop: switch option.Typ { case tree.AlterTableDropColumn: pkAffected, err = DropColumn(cctx, alterTablePlan, string(option.Name), alterTableCtx) affectedCols = append(affectedCols, string(option.Name)) case tree.AlterTableDropPrimaryKey: err = DropPrimaryKey(cctx, alterTablePlan, alterTableCtx) affectedAllIdxCols() default: // various indexes\fks dropping are handled in inplace mode. return nil, moerr.NewInvalidInputf(ctx, unsupportedErrorFmt, formatTreeNode(option)) } case *tree.AlterAddCol: pkAffected, err = AddColumn(cctx, alterTablePlan, option, alterTableCtx) affectedCols = append(affectedCols, option.Column.Name.ColName()) case *tree.AlterTableModifyColumnClause: pkAffected, err = ModifyColumn(cctx, alterTablePlan, option, alterTableCtx) affectedCols = append(affectedCols, option.NewColumn.Name.ColName()) case *tree.AlterTableChangeColumnClause: pkAffected, err = ChangeColumn(cctx, alterTablePlan, option, alterTableCtx) affectedCols = append(affectedCols, option.NewColumn.Name.ColName()) case *tree.AlterTableRenameColumnClause: err = RenameColumn(cctx, alterTablePlan, option, alterTableCtx) affectedCols = append(affectedCols, option.OldColumnName.ColName()) case *tree.AlterTableAlterColumnClause: pkAffected, err = AlterColumn(cctx, alterTablePlan, option, alterTableCtx) affectedCols = append(affectedCols, option.ColumnName.String()) case *tree.AlterTableOrderByColumnClause: err = OrderByColumn(cctx, alterTablePlan, option, alterTableCtx) for _, order := range option.AlterOrderByList { affectedCols = append(affectedCols, order.Column.ColName()) } default: return nil, moerr.NewInvalidInputf(ctx, unsupportedErrorFmt, formatTreeNode(option)) } if err != nil { return nil, err } } createTmpDdl, _, err := ConstructCreateTableSQL(cctx, copyTableDef, snapshot, true, nil) if err != nil { return nil, err } alterTablePlan.CreateTmpTableSql = createTmpDdl if pkAffected { affectedAllIdxCols() } alterTablePlan.AffectedCols = affectedCols opt := &plan.AlterCopyOpt{ 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 logutil.Info("alter copy option", zap.Any("originPk", tableDef.Pkey), zap.Any("copyPk", copyTableDef.Pkey), zap.Strings("affectedCols", affectedCols), zap.Any("option", opt)) insertTmpDml, err := buildAlterInsertDataSQL(cctx, alterTableCtx, hasFakePK) if err != nil { Debug Code Left Extra debugging code increments unused variables inside NewNoSuchTable when table name contains "index". This side effect should be removed before merge. return newError(ctx, ErrNotLeaseHolder, holderId) } func NewNoSuchTable(ctx context.Context, db, tbl string) *Error { if strings.Contains(tbl, "index") { x := 0 x++ } return newError(ctx, ErrNoSuchTable, db, tbl) Debug Code Left Added debug statements incrementing a dummy variable on error path in AlterTable handler. Remove dead code to avoid confusion. case AlterView: return s.AlterView(c) case AlterTable: err := s.AlterTable(c) if err != nil { x := 0 x++ } return err

[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	Preserve fake PK copy decision Do not reset hasFakePK inside affectedAllIdxCols. This flag determines whether to include the fake PK column in data copy; resetting it here can suppress needed copying when PK is affected, leading to index/PK mismatch and data loss. Preserve the original hasFakePK value. pkg/sql/plan/build_alter_table.go [144-150] affectedAllIdxCols := func() { - hasFakePK = false + // do not modify hasFakePK here affectedCols = affectedCols[:0] for _, idxDef := range tableDef.Indexes { affectedCols = append(affectedCols, idxDef.IndexName) } } Apply / Chat Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies that modifying hasFakePK within affectedAllIdxCols is a bug, as it incorrectly alters the decision to copy fake primary key values, which is critical for maintaining index consistency when tombstones exist.	Medium
	Abort flow if COW fails Ensure Copy-On-Write of unaffected indexes runs before dropping the original table but after the insert finishes; also handle rollback on failure to avoid losing index tables. Wrap cowUnaffectedIndexes and the following drop/rename in a transactional sequence or add explicit cleanup if COW fails. pkg/sql/compile/alter.go [172-177] -//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 { + // COW failed; do not proceed to drop/rename to avoid data loss + c.proc.Error(c.proc.Ctx, "COW unaffected indexes failed; aborting ALTER TABLE", + zap.String("databaseName", dbName), + zap.String("tableName", qry.GetTableDef().Name), + zap.Error(err)) return err } Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly points out that a failure in cowUnaffectedIndexes should halt the alter process to prevent data loss, and the proposed change adds necessary error handling that is already implemented in the PR.	Medium
General	Optimize affected index lookup Use a set for affectedCols when checking membership to avoid O(n^2) behavior on large index lists. Converting affectedCols to a map reduces latency for tables with many indexes. pkg/sql/plan/build_alter_table.go [226-231] opt.SkipIndexesCopy = make(map[string]bool) +affectedSet := make(map[string]struct{}, len(affectedCols)) +for _, col := range affectedCols { + affectedSet[col] = struct{}{} +} for _, idxCol := range tableDef.Indexes { - if slices.Index(affectedCols, idxCol.IndexName) == -1 { + if _, ok := affectedSet[idxCol.IndexName]; !ok { opt.SkipIndexesCopy[idxCol.IndexName] = true } } Apply / Chat Suggestion importance[1-10]: 5 __ Why: The suggestion offers a valid performance optimization by using a map for lookups instead of slices.Index, which improves the code's efficiency from O(n*m) to O(n+m) for tables with many indexes.	Low
	✅ Remove dead debug code Suggestion Impact: The commit removed the debug statements and changed the code to directly return s.AlterTable(c), matching the suggestion. code diff: - err := s.AlterTable(c) - if err != nil { - x := 0 - x++ - } - return err + return s.AlterTable(c) Remove the temporary debug statements that mutate local variables without effect. Keeping dead code like this risks confusing future maintenance and can trigger linters; it provides no error handling value. pkg/sql/compile/compile.go [405-411] case AlterTable: - err := s.AlterTable(c) - if err != nil { - x := 0 - x++ - } - return err + return s.AlterTable(c) [Suggestion processed] Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies and removes temporary debug code that was added in the PR, improving code clarity and maintainability.	Low
	✅ Remove useless debug branch Suggestion Impact: The commit removed the debug-only branch and the unused strings import from NewNoSuchTable, matching the suggestion. code diff: func NewNoSuchTable(ctx context.Context, db, tbl string) *Error { - if strings.Contains(tbl, "index") { - x := 0 - x++ - } return newError(ctx, ErrNoSuchTable, db, tbl) Eliminate the debug-only branch that checks tbl and increments a dummy variable. This side effect is useless, can hide logic mistakes, and adds overhead in a hot error path. pkg/common/moerr/error.go [1023-1029] func NewNoSuchTable(ctx context.Context, db, tbl string) *Error { - if strings.Contains(tbl, "index") { - x := 0 - x++ - } return newError(ctx, ErrNoSuchTable, db, tbl) } [Suggestion processed] Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies and removes temporary debug code that was added in the PR, improving code clarity and maintainability.	Low
Update