Explorar o código

fix: reject RENAME COLUMN that an expression index depends on

Expression-index definitions persist SQL text verbatim, so rewriting only
plain column references left a UNIQUE expression index bound to a column
name that no longer existed. Its indexed values resolved to NULL and
duplicates were silently accepted.

Detect the dependency by parsing the stored expression and refuse the rename
with an actionable error instead of leaving the constraint unenforced.

Also:
- treat rowid/oid/_rowid_ as valid ON CONFLICT targets instead of rejecting
  them as unknown columns
- write a table's changed index definitions in one batch so its indexes
  cannot split between the old and new shape
Danilo Fragoso hai 1 día
pai
achega
a461d0615b

+ 8 - 6
pkg/analyzer/analyzer.go

@@ -523,13 +523,15 @@ func (a *Analyzer) analyzeInsert(stmt *parser.InsertStmt) error {
 	}
 
 	// An ON CONFLICT target names columns of the target table. An unknown name
-	// must fail the statement rather than silently degrading to a scan.
+	// must fail the statement rather than silently degrading to a scan. The
+	// hidden rowid aliases are valid targets unless a real column shadows them.
 	for _, colName := range stmt.ConflictTarget {
-		if _, ok := table.GetColumn(colName); !ok {
-			return &AnalysisError{
-				Type:    ErrColumnNotFound,
-				Message: fmt.Sprintf("column not found: %s", colName),
-			}
+		if _, ok := table.GetColumn(colName); ok || isRowIDAlias(colName) {
+			continue
+		}
+		return &AnalysisError{
+			Type:    ErrColumnNotFound,
+			Message: fmt.Sprintf("column not found: %s", colName),
 		}
 	}
 

+ 1 - 1
pkg/capabilities/capabilities.go

@@ -179,7 +179,7 @@ func Current() *Document {
 			AddColumnBackfill:      Feature{Supported: false, Status: "no_backfill", Notes: "existing rows read NULL; a DEFAULT applies to later INSERTs only"},
 			DropColumn:             Feature{Supported: true, Status: "rewrites_rows"},
 			RenameColumn:           Feature{Supported: true, Status: "rewrites_rows"},
-			RenameColumnIndexes:    Feature{Supported: true, Status: "rewrites_index_metadata", Notes: "column references in index definitions are rewritten in place, including composite and unique indexes; index names are unchanged and expression-index SQL text is not rewritten"},
+			RenameColumnIndexes:    Feature{Supported: true, Status: "rewrites_index_metadata", Notes: "column references in index definitions are rewritten in place, including composite and unique indexes; index names are unchanged; RENAME COLUMN is rejected when an expression index references the column (drop that index, rename, then recreate it)"},
 			CreateTableIfNotExists: true,
 			AddColumnIfNotExists:   true,
 		},

+ 68 - 0
pkg/executor/alter_table_test.go

@@ -258,3 +258,71 @@ func TestAlterTableRenameColumnCycleRestoresIndexMetadata(t *testing.T) {
 		t.Fatalf("uq_a columns after rejected rename = %#v, want [a]", idx.Columns)
 	}
 }
+
+// A column referenced by an expression index cannot be renamed: the engine
+// stores expression text verbatim, so the rename is rejected instead of leaving
+// a UNIQUE expression index silently unbound from the column it enforces.
+func TestAlterTableRenameColumnRejectsExpressionIndexDependency(t *testing.T) {
+	_, schema, table := newTestDB(t)
+	e := newExec(schema, table)
+	execMust(t, e, "CREATE TABLE t (id INTEGER PRIMARY KEY, email TEXT, tag TEXT)")
+	execMust(t, e, "CREATE UNIQUE INDEX uq_lower_email ON t(lower(email))")
+	execMust(t, e, "INSERT INTO t (id, email, tag) VALUES (1, 'a@x', 'keep')")
+
+	_, err := execSQL(e, "ALTER TABLE t RENAME COLUMN email TO mail")
+	if err == nil {
+		t.Fatal("rename of a column used by an expression index must be rejected")
+	}
+	for _, want := range []string{"uq_lower_email", "lower(email)"} {
+		if !strings.Contains(err.Error(), want) {
+			t.Fatalf("rename error = %v, want it to name %q", err, want)
+		}
+	}
+
+	// Nothing was renamed, and the UNIQUE expression index still enforces.
+	sch, err := schema.GetSchema("t")
+	if err != nil {
+		t.Fatalf("get schema: %v", err)
+	}
+	if _, ok := sch.GetColumn("email"); !ok {
+		t.Fatal("rejected rename removed the email column")
+	}
+	if _, ok := sch.GetColumn("mail"); ok {
+		t.Fatal("rejected rename added the mail column")
+	}
+	if _, err := execSQL(e, "INSERT INTO t (id, email, tag) VALUES (2, 'a@x', 'dup')"); err == nil || !strings.Contains(err.Error(), "UNIQUE constraint failed") {
+		t.Fatalf("duplicate on expression index after rejected rename = %v, want UNIQUE constraint failure", err)
+	}
+
+	// A column the expression does not reference can still be renamed.
+	execMust(t, e, "ALTER TABLE t RENAME COLUMN tag TO label")
+	idx, err := schema.GetIndex("uq_lower_email")
+	if err != nil {
+		t.Fatalf("get uq_lower_email: %v", err)
+	}
+	if len(idx.Columns) != 1 || idx.Columns[0].Expression != "lower(email)" {
+		t.Fatalf("uq_lower_email columns = %#v, want the untouched lower(email) expression", idx.Columns)
+	}
+	if res := execMust(t, e, "SELECT label FROM t WHERE id = 1"); res.RowCount != 1 || res.Rows[0][0] != "keep" {
+		t.Fatalf("renamed label = %v, want keep", res.Rows)
+	}
+}
+
+// Conflict targets that name a hidden rowid alias are not table columns but are
+// still valid targets, including after a rename.
+func TestAlterTableRenameColumnKeepsRowIDConflictTarget(t *testing.T) {
+	_, schema, table := newTestDB(t)
+	e := newExec(schema, table)
+	execMust(t, e, "CREATE TABLE t (id INTEGER PRIMARY KEY, a TEXT)")
+	execMust(t, e, "INSERT INTO t (id, a) VALUES (1, 'x')")
+
+	execMust(t, e, "ALTER TABLE t RENAME COLUMN a TO b")
+
+	execMust(t, e, "INSERT INTO t (id, b) VALUES (2, 'y') ON CONFLICT (rowid) DO NOTHING")
+	execMust(t, e, "INSERT INTO t (id, b) VALUES (3, 'z') ON CONFLICT (oid) DO NOTHING")
+	execMust(t, e, "INSERT INTO t (id, b) VALUES (4, 'w') ON CONFLICT (_rowid_) DO NOTHING")
+
+	if res := execMust(t, e, "SELECT count(*) FROM t"); res.Rows[0][0] != int64(4) {
+		t.Fatalf("row count = %v, want 4", res.Rows[0][0])
+	}
+}

+ 33 - 0
pkg/executor/executor.go

@@ -4161,6 +4161,9 @@ func (e *Executor) executeAlterTableRename(table string, action *parser.RenameTa
 
 // executeAlterTableRenameColumn renames a column.
 func (e *Executor) executeAlterTableRenameColumn(table string, action *parser.RenameColumnAction) (*Result, error) {
+	if err := e.rejectExpressionIndexRename(table, action.OldName); err != nil {
+		return nil, err
+	}
 	if err := e.schema.RenameColumn(table, action.OldName, action.NewName); err != nil {
 		return nil, err
 	}
@@ -4172,6 +4175,36 @@ func (e *Executor) executeAlterTableRenameColumn(table string, action *parser.Re
 	return result, nil
 }
 
+// rejectExpressionIndexRename refuses to rename a column that a stored index
+// expression references. Expression text is persisted verbatim, so the rename
+// cannot rewrite it; leaving the old identifier behind would silently unbind a
+// UNIQUE expression index, because its rows no longer resolve the referenced
+// column.
+func (e *Executor) rejectExpressionIndexRename(table, column string) error {
+	indexes, err := e.schema.ListTableIndexes(table)
+	if err != nil {
+		return err
+	}
+	for _, idx := range indexes {
+		for _, ic := range idx.Columns {
+			if ic.Expression == "" {
+				continue
+			}
+			references, err := storedExpressionReferencesColumn(ic.Expression, column)
+			if err != nil {
+				return fmt.Errorf("index %s: %w", idx.Name, err)
+			}
+			if references {
+				return pgerror.Unsupported(
+					"cannot rename column %s: index %s depends on it through the expression %s; drop the index, rename the column, then recreate the index",
+					column, idx.Name, ic.Expression,
+				)
+			}
+		}
+	}
+	return nil
+}
+
 // Transaction execution methods
 
 // executeBegin starts a new transaction.

+ 16 - 0
pkg/executor/indexexpr.go

@@ -51,6 +51,22 @@ func EvalStoredExpression(text string, row storage.Row) (interface{}, error) {
 	return statelessEval.evalExpr(expr, row)
 }
 
+// storedExpressionReferencesColumn reports whether a persisted index expression
+// names column. The engine stores expression SQL text verbatim, so a column
+// rename cannot rewrite the reference.
+func storedExpressionReferencesColumn(text, column string) (bool, error) {
+	expr, err := parseStoredExpr(text)
+	if err != nil {
+		return false, err
+	}
+	for _, ref := range collectColumnRefs(expr) {
+		if strings.EqualFold(ref, column) {
+			return true, nil
+		}
+	}
+	return false, nil
+}
+
 // ValidateDeterministicExpr rejects an expression that SQLite would not allow in
 // an index or generated column: subqueries, window functions, aggregates, and
 // non-deterministic or unknown functions.

+ 36 - 11
pkg/storage/schema.go

@@ -1155,8 +1155,10 @@ func (m *SchemaManager) renameDataKeys(oldName, newName string) error {
 // updateIndexesOfTableLocked applies mutate to the durable definition of every
 // index on table and persists the definitions it changed. mutate reports
 // whether it modified the definition; unmodified indexes are left untouched.
-// The caller holds m.mu; it reads the durable index list directly rather than
-// calling the self-locking ListIndexes helper.
+// The changed definitions are written in one batch, so a table's indexes never
+// split between the old and new shape. The caller holds m.mu; it reads the
+// durable index list directly rather than calling the self-locking ListIndexes
+// helper.
 func (m *SchemaManager) updateIndexesOfTableLocked(table string, mutate func(*Index) bool) error {
 	var names []string
 	if m.indexListCached {
@@ -1179,6 +1181,12 @@ func (m *SchemaManager) updateIndexesOfTableLocked(table string, mutate func(*In
 		}
 	}
 
+	type indexUpdate struct {
+		index Index
+		key   string
+		value []byte
+	}
+	var updates []indexUpdate
 	for _, name := range names {
 		key := m.indexKey(name)
 		var data string
@@ -1207,13 +1215,26 @@ func (m *SchemaManager) updateIndexesOfTableLocked(table string, mutate func(*In
 		if err != nil {
 			return err
 		}
-		if err := m.pool.WithClient(func(c *KVClient) error {
-			return c.Write(key, string(updated))
-		}); err != nil {
-			return err
-		}
-		if cached, ok := m.indexCache[strings.ToLower(name)]; ok {
-			*cached = idx
+		updates = append(updates, indexUpdate{index: idx, key: key, value: updated})
+	}
+	if len(updates) == 0 {
+		return nil
+	}
+
+	ops := make([]BatchOp, 0, len(updates))
+	for _, update := range updates {
+		ops = append(ops, BatchOp{Op: batchPut, Key: []byte(update.key), Value: update.value})
+	}
+	if err := m.pool.WithClient(func(c *KVClient) error {
+		_, err := c.BatchWrite(ops, nil)
+		return err
+	}); err != nil {
+		return fmt.Errorf("update index definitions for table %s: %w", table, err)
+	}
+
+	for _, update := range updates {
+		if cached, ok := m.indexCache[strings.ToLower(update.index.Name)]; ok {
+			*cached = update.index
 		}
 	}
 	return nil
@@ -1231,7 +1252,9 @@ func (m *SchemaManager) repointIndexesLocked(oldName, newName string) error {
 // table so a reference to oldName becomes newName. Index names stay unchanged:
 // they are independent identifiers that SQLite does not derive from column
 // names, and callers may have chosen them explicitly. A rename to an existing
-// column name is rejected before this runs.
+// column name is rejected before this runs. Expression columns keep their SQL
+// text; callers must reject a rename that such an expression depends on,
+// because the text is not rewritten.
 func (m *SchemaManager) renameIndexColumnsLocked(table, oldName, newName string) error {
 	return m.updateIndexesOfTableLocked(table, func(idx *Index) bool {
 		changed := false
@@ -1247,7 +1270,9 @@ func (m *SchemaManager) renameIndexColumnsLocked(table, oldName, newName string)
 	})
 }
 
-// RenameColumn renames a column in a table.
+// RenameColumn renames a column in a table. Column references in index
+// definitions are rewritten; expression-index text is not, so a SQL caller must
+// reject a rename that an expression depends on.
 func (m *SchemaManager) RenameColumn(table, oldName, newName string) error {
 	tableLock := m.tableLock(table)
 	tableLock.Lock()