Prechádzať zdrojové kódy

feat: rewrite index metadata on ALTER TABLE RENAME COLUMN

RENAME COLUMN rewrote rows and the schema but left every stored index
definition naming the old column. Index metadata drives secondary-index
values, UNIQUE enforcement, ON CONFLICT target resolution, and catalog
introspection, so after a rename those paths either bypassed the index or
failed against a column that no longer existed.

RenameColumn now rewrites the definition of every index on the table,
including composite and unique indexes, while leaving index names unchanged
because they are independent identifiers. Expression-index SQL text is not
rewritten; the capability note documents that drop-and-recreate requirement.

ON CONFLICT targets are now validated against the target table, so the old
column name fails as an unknown column instead of surfacing a later UNIQUE
violation.

The index-definition walk is shared with RENAME TABLE's index repointing.
Danilo Fragoso 1 deň pred
rodič
commit
cbe47fb81b

+ 11 - 0
pkg/analyzer/analyzer.go

@@ -522,6 +522,17 @@ 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.
+	for _, colName := range stmt.ConflictTarget {
+		if _, ok := table.GetColumn(colName); !ok {
+			return &AnalysisError{
+				Type:    ErrColumnNotFound,
+				Message: fmt.Sprintf("column not found: %s", colName),
+			}
+		}
+	}
+
 	// Validate VALUES
 	for _, row := range stmt.Values {
 		if len(row) != len(targetCols) {

+ 1 - 0
pkg/analyzer/analyzer_test.go

@@ -383,6 +383,7 @@ func TestAnalyzeInsertErrors(t *testing.T) {
 	}{
 		{"table not found", "INSERT INTO nonexistent VALUES (1)", ErrTableNotFound},
 		{"column not found", "INSERT INTO users (nonexistent) VALUES (1)", ErrColumnNotFound},
+		{"unknown on conflict target", "INSERT INTO users (id, name) VALUES (1, 'a') ON CONFLICT (nonexistent) DO NOTHING", ErrColumnNotFound},
 		{"wrong column count", "INSERT INTO users (id, name) VALUES (1)", ErrTypeMismatch},
 	}
 

+ 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: false, Status: "stale_index_metadata", Notes: "drop and recreate indexes that reference a renamed column"},
+			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"},
 			CreateTableIfNotExists: true,
 			AddColumnIfNotExists:   true,
 		},

+ 3 - 0
pkg/capabilities/capabilities_test.go

@@ -39,4 +39,7 @@ func TestCurrentDocumentIsVersionedAndMarshalable(t *testing.T) {
 	if round.Notifications.PayloadLimit != 8000 || round.Notifications.DurableReplay {
 		t.Fatalf("unexpected notification limits: %+v", round.Notifications)
 	}
+	if !round.DDL.RenameColumnIndexes.Supported {
+		t.Fatal("RENAME COLUMN must advertise index metadata rewriting")
+	}
 }

+ 134 - 18
pkg/executor/alter_table_test.go

@@ -1,8 +1,11 @@
 package executor
 
 import (
+	"strings"
 	"sync"
 	"testing"
+
+	"github.com/danfragoso/pizzasql-next/pkg/storage"
 )
 
 // ADD COLUMN only appends schema metadata. Existing rows are not rewritten, so
@@ -112,33 +115,146 @@ func TestAlterTableDropColumnRewritesPopulatedRows(t *testing.T) {
 	}
 }
 
-// RENAME COLUMN rewrites row keys but deliberately leaves index metadata
-// unchanged. Documented guidance: drop and recreate indexes after a rename.
-func TestAlterTableRenameColumnLeavesIndexMetadataStale(t *testing.T) {
-	_, schema, table := newTestDB(t)
+// RENAME COLUMN rewrites the durable index definitions along with the schema
+// and rows, so single- and multi-column indexes name the new column while
+// keeping their original index names.
+func TestAlterTableRenameColumnUpdatesIndexMetadata(t *testing.T) {
+	pool, 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, "CREATE TABLE t (id INTEGER PRIMARY KEY, a TEXT, b TEXT)")
+	execMust(t, e, "INSERT INTO t (id, a, b) VALUES (1, 'x', 'y')")
 	execMust(t, e, "CREATE INDEX idx_a ON t(a)")
+	execMust(t, e, "CREATE INDEX idx_ab ON t(a, b)")
 
 	execMust(t, e, "ALTER TABLE t RENAME COLUMN a TO aa")
 
-	res := execMust(t, e, "SELECT aa FROM t WHERE id = 1")
-	if got := res.Rows[0][0]; got != "x" {
-		t.Fatalf("renamed value = %v, want x", got)
+	single, err := schema.GetIndex("idx_a")
+	if err != nil {
+		t.Fatalf("get idx_a: %v", err)
+	}
+	if len(single.Columns) != 1 || single.Columns[0].Name != "aa" {
+		t.Fatalf("idx_a columns = %#v, want [aa]", single.Columns)
 	}
 
-	names, err := schema.ListIndexes()
+	composite, err := schema.GetIndex("idx_ab")
 	if err != nil {
-		t.Fatalf("list indexes: %v", err)
+		t.Fatalf("get idx_ab: %v", err)
 	}
-	found := false
-	for _, name := range names {
-		if name == "idx_a" {
-			found = true
-		}
+	if len(composite.Columns) != 2 || composite.Columns[0].Name != "aa" || composite.Columns[1].Name != "b" {
+		t.Fatalf("idx_ab columns = %#v, want [aa b]", composite.Columns)
+	}
+
+	// index_info reports the renamed column and resolves its column id.
+	res := execMust(t, e, "PRAGMA index_info(idx_a)")
+	if len(res.Rows) != 1 || res.Rows[0][1] != int64(1) || res.Rows[0][2] != "aa" {
+		t.Fatalf("index_info(idx_a) = %v, want [[0 1 aa]]", res.Rows)
+	}
+
+	// sqlite_master renders the recreated index over the new column name.
+	res = execMust(t, e, "SELECT sql FROM sqlite_master WHERE type='index' AND name='idx_ab'")
+	if len(res.Rows) != 1 || !strings.Contains(res.Rows[0][0].(string), "(`aa`, `b`)") {
+		t.Fatalf("recreated idx_ab SQL = %v, want a definition over aa, b", res.Rows)
+	}
+
+	// The definition is durable, not just cached on this SchemaManager.
+	reloaded := storage.NewSchemaManager(pool, "testdb")
+	composite, err = reloaded.GetIndex("idx_ab")
+	if err != nil {
+		t.Fatalf("reload idx_ab: %v", err)
+	}
+	if len(composite.Columns) != 2 || composite.Columns[0].Name != "aa" || composite.Columns[1].Name != "b" {
+		t.Fatalf("reloaded idx_ab columns = %#v, want [aa b]", composite.Columns)
+	}
+
+	// Index-backed equality lookup still returns the row after the rename.
+	res = execMust(t, e, "SELECT id, aa FROM t WHERE aa = 'x'")
+	if res.RowCount != 1 || res.Rows[0][0] != int64(1) || res.Rows[0][1] != "x" {
+		t.Fatalf("indexed lookup after rename = %v, want [[1 x]]", res.Rows)
+	}
+}
+
+// A UNIQUE index keeps enforcing uniqueness after its column is renamed.
+func TestAlterTableRenameColumnKeepsUniqueIndexEnforcement(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_email ON t(email)")
+	execMust(t, e, "INSERT INTO t (id, email, tag) VALUES (1, 'a', 'first')")
+
+	if _, err := execSQL(e, "INSERT INTO t (id, email, tag) VALUES (2, 'a', 'dup')"); err == nil || !strings.Contains(err.Error(), "UNIQUE constraint failed") {
+		t.Fatalf("pre-rename duplicate error = %v, want UNIQUE constraint failure", err)
+	}
+
+	execMust(t, e, "ALTER TABLE t RENAME COLUMN email TO mail")
+
+	if _, err := execSQL(e, "INSERT INTO t (id, mail, tag) VALUES (3, 'a', 'dup')"); err == nil || !strings.Contains(err.Error(), "UNIQUE constraint failed") {
+		t.Fatalf("post-rename duplicate error = %v, want UNIQUE constraint failure", err)
+	}
+	execMust(t, e, "INSERT INTO t (id, mail, tag) VALUES (4, 'b', 'ok')")
+
+	res := execMust(t, e, "SELECT id, mail, tag FROM t ORDER BY id")
+	if res.RowCount != 2 || res.Rows[0][1] != "a" || res.Rows[1][1] != "b" {
+		t.Fatalf("rows after rename = %v, want two distinct emails", res.Rows)
+	}
+
+	res = execMust(t, e, "PRAGMA index_list(t)")
+	if len(res.Rows) != 1 || res.Rows[0][1] != "uq_email" || res.Rows[0][2] != int64(1) {
+		t.Fatalf("index_list(t) = %v, want the unique uq_email index", res.Rows)
+	}
+}
+
+// ON CONFLICT resolves against the renamed column and rejects the old name.
+func TestAlterTableRenameColumnResolvesOnConflictTarget(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_email ON t(email)")
+	execMust(t, e, "INSERT INTO t (id, email, tag) VALUES (1, 'a', 'old')")
+
+	execMust(t, e, "ALTER TABLE t RENAME COLUMN email TO mail")
+
+	execMust(t, e, "INSERT INTO t (id, mail, tag) VALUES (2, 'a', 'new') ON CONFLICT (mail) DO UPDATE SET tag = excluded.tag")
+	res := execMust(t, e, "SELECT id, mail, tag FROM t")
+	if res.RowCount != 1 || res.Rows[0][0] != int64(1) || res.Rows[0][2] != "new" {
+		t.Fatalf("upsert on renamed column = %v, want the existing row updated", res.Rows)
+	}
+
+	if _, err := execSQL(e, "INSERT INTO t (id, mail, tag) VALUES (3, 'a', 'x') ON CONFLICT (email) DO UPDATE SET tag = excluded.tag"); err == nil || !strings.Contains(err.Error(), "column not found: email") {
+		t.Fatalf("upsert on old column error = %v, want column not found: email", err)
+	}
+}
+
+// Renaming a column back restores the original index metadata, and renaming to
+// an existing column name still fails without touching that metadata.
+func TestAlterTableRenameColumnCycleRestoresIndexMetadata(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, "CREATE UNIQUE INDEX uq_a ON t(a)")
+	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, "ALTER TABLE t RENAME COLUMN b TO a")
+
+	idx, err := schema.GetIndex("uq_a")
+	if err != nil {
+		t.Fatalf("get uq_a: %v", err)
+	}
+	if len(idx.Columns) != 1 || idx.Columns[0].Name != "a" {
+		t.Fatalf("uq_a columns after cycle = %#v, want [a]", idx.Columns)
+	}
+	if _, err := execSQL(e, "INSERT INTO t (id, a) VALUES (2, 'x')"); err == nil || !strings.Contains(err.Error(), "UNIQUE constraint failed") {
+		t.Fatalf("duplicate error after rename cycle = %v, want UNIQUE constraint failure", err)
+	}
+
+	if _, err := execSQL(e, "ALTER TABLE t RENAME COLUMN a TO id"); err == nil || !strings.Contains(err.Error(), "column already exists: id") {
+		t.Fatalf("rename to existing column error = %v, want column already exists: id", err)
+	}
+	idx, err = schema.GetIndex("uq_a")
+	if err != nil {
+		t.Fatalf("get uq_a after rejected rename: %v", err)
 	}
-	if !found {
-		t.Fatalf("indexes = %v, want idx_a to still exist (rename does not rewrite index metadata)", names)
+	if len(idx.Columns) != 1 || idx.Columns[0].Name != "a" {
+		t.Fatalf("uq_a columns after rejected rename = %#v, want [a]", idx.Columns)
 	}
 }

+ 51 - 9
pkg/storage/schema.go

@@ -1152,10 +1152,12 @@ func (m *SchemaManager) renameDataKeys(oldName, newName string) error {
 	})
 }
 
-// repointIndexesLocked updates indexes whose table was renamed. The caller holds
-// m.mu; it reads the durable index list directly rather than calling the
-// self-locking ListIndexes helper.
-func (m *SchemaManager) repointIndexesLocked(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.
+func (m *SchemaManager) updateIndexesOfTableLocked(table string, mutate func(*Index) bool) error {
 	var names []string
 	if m.indexListCached {
 		names = append([]string(nil), m.indexListCache...)
@@ -1195,10 +1197,12 @@ func (m *SchemaManager) repointIndexesLocked(oldName, newName string) error {
 		if err := json.Unmarshal([]byte(data), &idx); err != nil {
 			return err
 		}
-		if !strings.EqualFold(idx.Table, oldName) {
+		if !strings.EqualFold(idx.Table, table) {
+			continue
+		}
+		if !mutate(&idx) {
 			continue
 		}
-		idx.Table = newName
 		updated, err := json.Marshal(&idx)
 		if err != nil {
 			return err
@@ -1209,12 +1213,40 @@ func (m *SchemaManager) repointIndexesLocked(oldName, newName string) error {
 			return err
 		}
 		if cached, ok := m.indexCache[strings.ToLower(name)]; ok {
-			cached.Table = newName
+			*cached = idx
 		}
 	}
 	return nil
 }
 
+// repointIndexesLocked updates indexes whose table was renamed.
+func (m *SchemaManager) repointIndexesLocked(oldName, newName string) error {
+	return m.updateIndexesOfTableLocked(oldName, func(idx *Index) bool {
+		idx.Table = newName
+		return true
+	})
+}
+
+// renameIndexColumnsLocked rewrites the stored definition of every index on
+// 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.
+func (m *SchemaManager) renameIndexColumnsLocked(table, oldName, newName string) error {
+	return m.updateIndexesOfTableLocked(table, func(idx *Index) bool {
+		changed := false
+		for i := range idx.Columns {
+			// Expression columns persist SQL text, not a column reference; their
+			// text is left for the caller to drop and recreate the index.
+			if idx.Columns[i].Expression == "" && strings.EqualFold(idx.Columns[i].Name, oldName) {
+				idx.Columns[i].Name = newName
+				changed = true
+			}
+		}
+		return changed
+	})
+}
+
 // RenameColumn renames a column in a table.
 func (m *SchemaManager) RenameColumn(table, oldName, newName string) error {
 	tableLock := m.tableLock(table)
@@ -1268,8 +1300,18 @@ func (m *SchemaManager) RenameColumn(table, oldName, newName string) error {
 		return err
 	}
 
-	// Update schema
-	return m.updateSchemaUnsafe(schema)
+	// Publish the renamed schema before the index definitions: reads of the new
+	// column name resolve against rewritten rows as soon as the schema lands.
+	// Index DDL is not transactional (as elsewhere in this engine), so a failure
+	// here leaves stale index metadata for the caller to drop and recreate.
+	if err := m.updateSchemaUnsafe(schema); err != nil {
+		return err
+	}
+
+	// Index metadata drives secondary-index values, UNIQUE enforcement,
+	// ON CONFLICT target resolution, and catalog introspection, so it must name
+	// the renamed column before any later statement uses the table.
+	return m.renameIndexColumnsLocked(table, oldName, newName)
 }
 
 // rewriteRows applies an idempotent name-keyed row transformation in bounded