DB Manager alter table

[diegoimbert]

Screen.Recording.2026-01-06.at.20.37.35.mov

Works well on Postgres (and consequently Data tables) and Ducklake. It is totally usable in all other databases too (except Oracle), but not as exhaustively tested.

The E2E tests PR will allow to guarantee best quality on all databases (there are a lot of edge cases across databases that are difficult to keep track of manually)

Transactional DDL is only supported on Postgres, MS SQL and DuckDB / Ducklake. I display a warning on other databases that any failure might leave the db in an intermediate state.

Columns are "stateful" as in renaming a column a to b is not the same as deleting a then creating b.

TODO : Better UI feedback for some restrictions (e.g: BigQuery can only drop NOT NULL, never add it. Or : it can never add a NOT NULL column to an existing table for data consistency reasons)

TODO : column reordering

[cloudflare-workers-and-pages]

Deploying windmill with    Cloudflare Pages

Latest commit:	dd0b4e3
Status:	✅  Deploy successful!
Preview URL:	https://5c6f908b.windmill.pages.dev
Branch Preview URL:	https://di-db-manager-update-table.windmill.pages.dev

View logs

[diegoimbert]

/ai review this pr

[claude]

Claude finished @diegoimbert's task in 1m 34s —— View job

---

PR Review: DB Manager Alter Table

• Gather context and read changed files
• Review code for bugs, issues, and improvements
• Post comprehensive review feedback

---

Summary

This PR adds ALTER TABLE functionality to the DB Manager, enabling users to modify existing database tables (add/drop/alter columns, manage foreign keys, manage primary keys, and rename tables). The implementation supports multiple database types (PostgreSQL, MySQL, MS SQL Server, Snowflake, BigQuery, DuckDB).

Overall Assessment

✅ Well-structured implementation with good separation of concerns
✅ Comprehensive test coverage (1000+ lines of tests)
✅ Good multi-database support with database-specific SQL generation
✅ Smart operation ordering to avoid dependency issues

---

Code Review Findings

🟢 Positives

1. Excellent test coverage - alterTable.test.ts covers many edge cases including column renames with FK references, PK changes, and operation ordering.

2. Well-designed diffing algorithm - diffTableEditorValues() correctly handles:

   • Column renames via initialName tracking
   • FK normalization when source columns are renamed
   • PK change detection with proper constraint name handling

3. Smart operation ordering in sortOperations() - Properly orders operations to avoid dependency issues (drop FKs first, then PKs, then columns, etc.)

4. Transactional DDL support - Wraps operations in transactions where supported (PostgreSQL, MS SQL Server, DuckDB)

5. Good type safety with well-defined TypeScript types

🟡 Suggestions for Improvement

1. MS SQL Server Default Constraint Naming (alterTable.ts:240)

return `ALTER TABLE ${tableRef} ADD CONSTRAINT DF_${tableRef}_${columnName} DEFAULT ${defaultValue} FOR ${columnName};`

The constraint name DF_${tableRef}_${columnName} could contain dots if tableRef includes a schema (e.g., DF_public.users_email). This might cause issues. Consider sanitizing or using underscores:

return `ALTER TABLE ${tableRef} ADD CONSTRAINT DF_${tableRef.replace('.', '_')}_${columnName} DEFAULT ${defaultValue} FOR ${columnName};`

Fix this →

2. Potential Issue with Empty Operations (alterTable.ts:74-82)

When makeAlterTableQuery receives no operations, it returns an empty string. However, makeAlterTableQueries is called first which could return an empty array. The transactional wrapping logic should handle this case more gracefully to avoid generating BEGIN;\n\nCOMMIT; queries.

Current code handles this correctly by checking if (queries.length === 0) return '' first, but it's worth noting for clarity.

3. Missing Identifier Quoting in Some Places

In alterTable.ts, column names and table names in some ALTER statements aren't consistently quoted. For example:

• Line 98: renderColumn doesn't quote the column name
• Line 102: DROP COLUMN ${op.name} - op.name isn't quoted
• Line 124: RENAME TO ${op.to} - op.to isn't quoted

While you mentioned sanitization is planned for a future PR, this is worth tracking. Identifiers with spaces or reserved words will fail.

4. FK Constraint Name Generation (dbQueriesUtils.ts:49-51)

sql += `CONSTRAINT fk_${[options.tableName, ...sourceColumns, targetTable, ...targetColumns]
    .join('_')
    .replaceAll('.', '_')} `

This could generate very long constraint names (>63 chars for PostgreSQL). Consider truncating or hashing:

const constraintName = `fk_${[options.tableName, ...sourceColumns, targetTable, ...targetColumns]
    .join('_')
    .replaceAll('.', '_')
    .substring(0, 60)}`

5. Test Assertion Typo (alterTable.test.ts:200-203)

// MySQL test expects DROP CONSTRAINT but should expect DROP FOREIGN KEY based on renderDropForeignKey
expect(normalize(queries[0])).toBe(
    normalize('ALTER TABLE posts DROP CONSTRAINT fk_posts_user;')
)

Looking at renderDropForeignKey (line 294), MySQL should use DROP FOREIGN KEY, but the test expects DROP CONSTRAINT. This test may be incorrect.

Fix this →

6. dbSupportsSchemas for MS SQL Server (utils.ts:376-378)

export function dbSupportsSchemas(dbType: DbType): boolean {
    return dbType === 'postgresql' || dbType === 'snowflake' || dbType === 'bigquery'
}

MS SQL Server also supports schemas (e.g., dbo.tablename). This might affect the tableRef generation in makeAlterTableQueries. Currently MS SQL operations might not include the schema prefix correctly.

🔴 Potential Issues

1. Default Value Drop Without Original Value Check (alterTable.ts:160-168)

if ('defaultValue' in changes) {
    if (!changes.defaultValue && original.defaultValue) {
        // Drop default
    } else if (changes.defaultValue) {
        // Set default
    }
}

The condition !changes.defaultValue will be true for both undefined and '' (empty string). If someone explicitly sets defaultValue: '', it will drop the default instead of setting it to an empty string. Consider using explicit === undefined check.

2. Missing Error Handling in fetchTableRelationalKeys (relationalKeys.ts:408-438)

The function silently catches errors and returns empty results:

} catch (e) {
    console.warn('Failed to fetch foreign keys:', e)
}

While this prevents crashes, users might not know why their FK data is missing. Consider surfacing these errors more visibly in the UI.

---

Architecture Notes

The PR follows good patterns from the existing codebase:

• Uses existing query building patterns from createTable.ts
• Properly extends IDbSchemaOps interface
• Integrates well with the existing DBManager and DBTableEditor components

---

Checklist

• ✅ Code compiles and type-checks
• ✅ Tests are comprehensive and cover edge cases
• ✅ Multi-database support implemented
• ✅ UI integration complete
• ⚠️ Some identifier quoting inconsistencies (acknowledged for future PR)
• ⚠️ One test case may have incorrect expectation (MySQL DROP FOREIGN KEY)

---

Overall: Good PR with solid implementation. The suggestions above are mostly minor improvements. The core functionality appears correct and well-tested.

[claude]

Claude finished @diegoimbert's task in 2m 25s —— View job

---

PR Review: DB Manager Alter Table

• Analyze PR changes and gather context
• Review code quality and best practices
• Check for potential bugs or issues
• Evaluate performance considerations
• Assess security implications
• Provide detailed feedback with inline comments
• Add testing instructions

---

Summary

This PR adds ALTER TABLE functionality to the DB Manager, enabling users to modify existing database tables including:

• Add/drop/alter columns
• Manage foreign keys (add/drop)
• Manage primary keys (add/drop)
• Rename tables

The implementation supports multiple database types: PostgreSQL, MySQL, MS SQL Server, Snowflake, BigQuery, and DuckDB.

---

Overall Assessment

✅ Well-structured implementation with good separation of concerns between SQL generation, diffing logic, and UI
✅ Comprehensive test coverage (1000+ lines of tests covering edge cases)
✅ Good multi-database support with database-specific SQL generation
✅ Smart operation ordering in sortOperations() to avoid dependency issues
✅ Clean Svelte 5 patterns using $state, $derived, and proper reactivity

---

Code Quality Highlights

Positives

1. Well-designed diffing algorithm - diffTableEditorValues() correctly handles:

   • Column renames via initialName tracking
   • FK normalization when source columns are renamed (see normalizeNewFkToOldColNames)
   • PK change detection with proper constraint name handling

2. Smart operation ordering (alterTable.ts:437-451) - Properly orders operations:

   1. Drop FKs first (they depend on columns)
   2. Drop PKs
   3. Drop columns
   4. Alter columns
   5. Add columns
   6. Add PKs
   7. Add FKs
   8. Rename table (last to avoid reference confusion)

3. Transactional DDL support - Wraps operations in transactions where supported (PostgreSQL, MS SQL Server, DuckDB) with appropriate warning for non-transactional databases

4. Good type safety - Well-defined TypeScript types in tableEditor.ts and alterTable.ts

5. Code reuse - Extracted renderColumn and renderForeignKey into dbQueriesUtils.ts for sharing between create and alter operations

---

Issues Found

See inline comments for specific code locations. Summary:

🟡 Medium Priority

1. Inconsistent identifier quoting - Column names in some ALTER statements aren't consistently quoted. While the PR author acknowledges this will be addressed in a future PR for sanitization, identifiers with spaces or reserved keywords will currently fail.

2. Silent FK fetch errors (relationalKeys.ts:436) - Errors are caught and logged to console only, users won't know why FK data is missing.

3. MS SQL Server schema support - dbSupportsSchemas() returns false for MS SQL Server, but it does support schemas (dbo.tablename). This might affect tableRef generation.

🟢 Minor/Suggestions

4. Empty string default value handling (alterTable.ts:165) - The truthiness check changes.defaultValue won't handle empty string defaults correctly.

5. Long FK constraint names - Generated constraint names could exceed PostgreSQL's 63-character limit.

---

Architecture Notes

The PR follows good patterns from the existing codebase:

• Uses existing query building patterns from createTable.ts
• Properly extends IDbSchemaOps interface with onAlter and previewAlterSql
• Integrates well with the existing DBManager and DBTableEditor components
• Clean separation: SQL generation in queries/, types in tableEditor.ts, UI in DBTableEditor.svelte

---

Security Considerations

⚠️ SQL Injection Note: As mentioned by the PR author, the code doesn't sanitize identifiers. This is acceptable since:

• Users with access to alter tables already have full database access
• This is admin-level functionality
• The PR author plans to address sanitization in a future PR for better handling of spaces and special characters

---

Testing Instructions

To test the ALTER TABLE functionality:

Navigate to the DB Manager page (accessible from the sidebar in workspaces with database resources configured). Select a database resource (PostgreSQL or Data Tables work best for testing). In the table list on the left sidebar, click the three-dot menu on any existing table and select "Alter table". A drawer will open showing the current table structure with the table name field (editable to rename), columns table with type/primary key/settings, and foreign keys section. Try renaming columns (note the "Old name" hint), changing types, toggling nullable/primary key, adding/deleting columns, and managing foreign keys. Click "Alter table" to see the SQL preview in a confirmation modal. Note the warning on non-transactional databases. Verify the button shows "No changes detected" when unmodified and "Alter table (X changes detected)" when changed.

---

[claude]

Potential issue with setting default values: The condition changes.defaultValue uses truthiness check which means an empty string '' would be falsy and not trigger setting the default. If a user explicitly wants to set an empty string as the default value, this won't work as expected.

Consider being more explicit:

} else if (changes.defaultValue !== undefined) {

[claude]

Silent error handling: The catch block silently catches errors and only logs a warning. While this prevents crashes, users won't know why their FK data is missing. Consider surfacing these errors more visibly in the UI, perhaps by returning an error field that can be displayed to users.

[claude]

Long FK constraint names: The generated constraint name could exceed PostgreSQL's 63-character limit for identifiers. Consider adding a truncation safeguard:

Suggested change

	sql += `CONSTRAINT fk_${[
	sql += `CONSTRAINT ${`fk_${[
	options.tableName,
	...sourceColumns.map((c) => c?.substring(0, 10)),
	targetTable,
	...targetColumns.map((c) => c?.substring(0, 10))
	]
	.join('_')
	.replaceAll('.', '_')}`.substring(0, 63)} `

Note: The .substring(0, 60) on line 54 already limits length, but applying it to the whole constraint name rather than at the concatenation level would be cleaner.