Support ALTER TABLE ... ALTER COLUMN ... TYPE ... USING <expr> - #3154
Support ALTER TABLE ... ALTER COLUMN ... TYPE ... USING <expr>#3154zachmu wants to merge 1 commit into
Conversation
Fixes #3119. The parser already accepted the USING clause, but the AST conversion rejected it. This adds a dedicated AlterTableColumnTypeUsing node (injected via vitess.InjectedStatement) that: - resolves the USING expression against the table with dolt's expranalysis.ResolveExpression (Postgres syntax, per-row evaluation) - applies an assignment cast from the expression's type to the new column type, matching Postgres semantics - rewrites the table through sql.RewritableTable.RewriteInserter, so primary keys and secondary indexes are rebuilt for the new type - enforces NOT NULL on the computed values and rejects type changes for columns used by foreign keys, mirroring the plain type-change path - honors IF EXISTS and reuses the row-type-usage validation from the MODIFY COLUMN pre-hook (extracted into an exported helper) The USING form is only supported as the sole action of an ALTER TABLE statement; combining it with other actions returns a clear error.
|
|
SummaryCoverage spans successful and rejected data-type changes, value conversion, schema qualification, key and index integrity, dependency handling, rollback and recovery, concurrency, and adversarial constraint cases. Most behaviors remain healthy, but failed conversions are not reliably atomic and can corrupt existing stored data while leaving the table writable. Not safe to merge yet — this PR has an attributable high-severity data-integrity failure: a late conversion error can expose partial schema and rewrite state, making existing rows and indexed access disappear. The documented constraint behavior is a separate by-design caveat, but it does not offset the merge-blocking corruption risk. Tests run by ItoTip Reply with @itoqa to send us feedback on this test run. |
| oldPkSchema := sql.SchemaToPrimaryKeySchema(ctx, rwt, sch) | ||
| newPkSchema := sql.SchemaToPrimaryKeySchema(ctx, rwt, newSch) | ||
|
|
||
| inserter, err := rwt.RewriteInserter(ctx, oldPkSchema, newPkSchema, oldCol, &newCol, nil) |
There was a problem hiding this comment.
Late conversion failure loses table data
What failed: The conversion reported a failure, but the table was left with integer type metadata, no original rows available through the index, and a row count of zero in the recorded SQL output. The follow-up insert succeeded, which shows the table was usable but still corrupted from the failed conversion.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: High
- Impact: A failed column conversion can leave the table with missing rows and the wrong column type. Users may lose access to stored records even though the table still accepts new writes.
- Steps to Reproduce:
- Create a table with an integer primary key, a text value column, a secondary index, and many valid numeric text values followed by one value such as 'not-a-number'.
- Run ALTER TABLE late_failure_rows ALTER COLUMN value TYPE integer USING value::integer.
- After the conversion fails, inspect the value column type, count the rows, and query the original rows through the secondary index.
- Try a normal insert and read it back to confirm that the table accepts new data even though the original data was not restored.
- Stub / mock content: No stubs, mocks, or bypasses were applied for this test in the recorded run.
- Code Analysis: AlterTableColumnTypeUsing.RowIter creates a RewriteInserter with the old and new primary-key schemas at server/node/alter_table_column_type_using.go:190-196, then scans the source table and inserts each converted row as it goes at lines 203-234. A conversion error from Eval or NewType.Convert at lines 212-220, a NOT NULL error at lines 223-225, or an insert error at lines 231-232 calls abortRewrite. The PR-added abortRewrite implementation at lines 275-279 calls inserter.DiscardChanges(ctx, err) and inserter.Close(ctx) but intentionally ignores both returned errors before returning only the original conversion error. Therefore, if the storage engine cannot discard or close the staged rewrite cleanly, the caller receives the expected conversion error while the incremental physical changes remain visible. The recorded result matches this path: the value column became integer, the original rows and indexed lookups disappeared, and a new insert worked afterward. The smallest practical fix is to preserve the original conversion error while detecting cleanup failure and ensuring the RewriteInserter rollback/close path either completes atomically or surfaces an explicit unrecoverable cleanup error without exposing the partial rewrite.
- Why this is likely a bug: A failed schema conversion must leave the original table, rows, primary key, and secondary indexes intact; otherwise one bad value can cause data loss. The test placed the invalid value after many valid rows and observed changed type metadata and missing indexed rows, which is the direct harmful outcome of exposing incremental rewrite state after cleanup fails. This is not a setup-only issue: the application was responsible for the rewrite and the relevant implementation was introduced by this PR. The targeted fix is to make failed rewrites atomic and to handle cleanup errors instead of silently discarding them.
Relevant code
server/node/alter_table_column_type_using.go:193-234
inserter, err := rwt.RewriteInserter(ctx, oldPkSchema, newPkSchema, oldCol, &newCol, nil)
...
rowIter := sql.NewTableRowIter(ctx, rwt, partitions)
for {
row, err := rowIter.Next(ctx)
...
newVal, err := conversionExpr.Eval(ctx, row)
...
if err != nil {
return nil, abortRewrite(ctx, inserter, err)
}
...
if err = inserter.Insert(ctx, newRow); err != nil {
return nil, abortRewrite(ctx, inserter, err)
}
}server/node/alter_table_column_type_using.go:275-279
func abortRewrite(ctx *sql.Context, inserter sql.RowInserter, err error) error {
_ = inserter.DiscardChanges(ctx, err)
_ = inserter.Close(ctx)
return err
}Evidence Package
Copy prompt for an agent
Ito QA identified the following failure during automated PR testing. Please investigate and propose a fix.
**High severity — Late conversion failure loses table data**
**What failed:** The conversion reported a failure, but the table was left with integer type metadata, no original rows available through the index, and a row count of zero in the recorded SQL output. The follow-up insert succeeded, which shows the table was usable but still corrupted from the failed conversion.
- **Impact:** A failed column conversion can leave the table with missing rows and the wrong column type. Users may lose access to stored records even though the table still accepts new writes.
- **Steps to reproduce:**
1. Create a table with an integer primary key, a text value column, a secondary index, and many valid numeric text values followed by one value such as 'not-a-number'.
2. Run ALTER TABLE late_failure_rows ALTER COLUMN value TYPE integer USING value::integer.
3. After the conversion fails, inspect the value column type, count the rows, and query the original rows through the secondary index.
4. Try a normal insert and read it back to confirm that the table accepts new data even though the original data was not restored.
- **Stub / mock content:** No stubs, mocks, or bypasses were applied for this test in the recorded run.
- **Code analysis:** AlterTableColumnTypeUsing.RowIter creates a RewriteInserter with the old and new primary-key schemas at server/node/alter_table_column_type_using.go:190-196, then scans the source table and inserts each converted row as it goes at lines 203-234. A conversion error from Eval or NewType.Convert at lines 212-220, a NOT NULL error at lines 223-225, or an insert error at lines 231-232 calls abortRewrite. The PR-added abortRewrite implementation at lines 275-279 calls inserter.DiscardChanges(ctx, err) and inserter.Close(ctx) but intentionally ignores both returned errors before returning only the original conversion error. Therefore, if the storage engine cannot discard or close the staged rewrite cleanly, the caller receives the expected conversion error while the incremental physical changes remain visible. The recorded result matches this path: the value column became integer, the original rows and indexed lookups disappeared, and a new insert worked afterward. The smallest practical fix is to preserve the original conversion error while detecting cleanup failure and ensuring the RewriteInserter rollback/close path either completes atomically or surfaces an explicit unrecoverable cleanup error without exposing the partial rewrite.
- **Why this is likely a bug:** A failed schema conversion must leave the original table, rows, primary key, and secondary indexes intact; otherwise one bad value can cause data loss. The test placed the invalid value after many valid rows and observed changed type metadata and missing indexed rows, which is the direct harmful outcome of exposing incremental rewrite state after cleanup fails. This is not a setup-only issue: the application was responsible for the rewrite and the relevant implementation was introduced by this PR. The targeted fix is to make failed rewrites atomic and to handle cleanup errors instead of silently discarding them.
**Relevant code:**
`server/node/alter_table_column_type_using.go:193-234`
~~~go
inserter, err := rwt.RewriteInserter(ctx, oldPkSchema, newPkSchema, oldCol, &newCol, nil)
...
rowIter := sql.NewTableRowIter(ctx, rwt, partitions)
for {
row, err := rowIter.Next(ctx)
...
newVal, err := conversionExpr.Eval(ctx, row)
...
if err != nil {
return nil, abortRewrite(ctx, inserter, err)
}
...
if err = inserter.Insert(ctx, newRow); err != nil {
return nil, abortRewrite(ctx, inserter, err)
}
}
~~~
`server/node/alter_table_column_type_using.go:275-279`
~~~go
func abortRewrite(ctx *sql.Context, inserter sql.RowInserter, err error) error {
_ = inserter.DiscardChanges(ctx, err)
_ = inserter.Close(ctx)
return err
}
~~~|
@zachmu DOLT
|

Adds support for ALTER TABLE ... ALTER COLUMN ... TYPE ... USING .
Fixes #3119.