feat: Allow reordering columns of empty tables during auto-migration - #5694
Open
Ludv1gL wants to merge 1 commit into
Open
feat: Allow reordering columns of empty tables during auto-migration#5694Ludv1gL wants to merge 1 commit into
Ludv1gL wants to merge 1 commit into
Conversation
A table with no resident rows has no data to migrate, so a layout-incompatible reschema is safe — the same justification already encoded for event tables. Reordering previously always failed with `AutoMigrateError::ReorderTable`. - Add `AutoMigratePrecheck::CheckTableEmpty`, validating emptiness before any mutations (as requested in the clockworklabs#4875 review), and use it for `RemoveTable` and the new step. - Add `AutoMigrateStep::ReschemaEmptyTable`; the planner emits it (plus the precheck and `DisconnectAllUsers`) for column position changes on non-event tables. Sub-objects on moved columns are removed and re-added; changed unique constraints are allowed (Remove+Add) only under this step. - Generalize `alter_event_table_row_type` into `alter_empty_table_row_type`; rename `PendingSchemaChange::ReschemaEventTable` -> `ReschemaEmptyTable` and `TableError::EventTableNotEmpty` -> `TableNotEmpty`. - Fix replay: `st_column_changed` now uses the unchecked empty-table path when the table is empty at that point in the log, so a committed reorder replays instead of failing layout checks on reopen. - Fix `change_columns_of_empty_table_to` on tables that previously contained rows: drop residual pages before `set_pages` (new `Pages::reset`). - Update the automatic-migrations docs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of Changes
Follow-up to #4593 (allow dropping empty tables): this allows reordering the columns of an empty table during an automatic migration, and implements the
AutoMigratePrecheck::CheckTableEmptyrefactor requested in the review of #4875 (#4875 (comment)).The justification is the one already encoded for event tables in
auto_migrate_table: a table with no resident rows has no data to migrate, so a layout-incompatible reschema is safe. Previously, reordering columns always failed withAutoMigrateError::ReorderTable, and the only way to reorder was to drop the (empty) table in one publish and re-add it in a second one, losing its identity.Design
Planner (
crates/schema/src/auto_migrate.rs):AutoMigratePrecheck::CheckTableEmpty(table): validates at execution time — before any mutations — that the table has no rows. Emitted forRemoveTable(moving the check out of the step executor, per the feat: Allow toggling event flag during auto-migration for empty tables #4875 review) and for the new reschema step.AutoMigrateStep::ReschemaEmptyTable(table): a layout-incompatible reschema of a non-event table. Declared between theRemove*andAdd*variants so that step sorting brackets it correctly with the sub-object steps below.columns_reorderedflag (4th slot of theArrayMonoid) instead of erroring; the table then getsCheckTableEmpty+ReschemaEmptyTable+DisconnectAllUsers.AutoMigrateError::ReorderTableis removed. Type changes must still be upgradable, and the column add/remove rules are unchanged.Remove*+Add*when their column ids change (the stale "column ids are not changed in an automigrate" comment is updated), andChangePrimaryKeycovers primary key moves. Changed unique constraints previously always errored withChangeUniqueConstraint; they now diff toRemoveConstraint+AddConstraint, but only when the owning table has aReschemaEmptyTablestep — on an empty table re-adding the constraint is trivially valid. All other tables keep the error.Executor (
crates/engine/src/update.rs):CheckTableEmptyprecheck: fails the publish with a clear message (table name + row count + how to fix) before any mutations.ReschemaEmptyTablestep: rebuilds the table's column layout from the new definition via a newRelationalDB::alter_empty_table_row_type.RemoveTablearm's inline row-count check is removed (subsumed by the precheck).Datastore:
MutTxId::alter_event_table_row_typeis refactored into a generalalter_empty_table_row_type(runtime emptiness check kept as a backstop); the event variant remains as a wrapper that keeps its is-an-event-table sanity check.PendingSchemaChange::ReschemaEventTable→ReschemaEmptyTableandTableError::EventTableNotEmpty→TableNotEmpty, since both now cover non-event tables too. The rollback path is unchanged — its safety argument ("the commit table is empty") already holds.replay.rs,st_column_changed): replay applied non-eventst_columnchanges with the layout-checkedchange_columns_to, which rejects a reorder — so a database that committed a reorder would fail to reopen from the commitlog. Replay now takes the unchecked empty-table path whenever the table has no rows at that point in the log, mirroring the condition under which such a change can be committed.Table (
crates/table):Table::change_columns_of_empty_table_tocalledset_pages(Vec::new(), ..), whosePages::set_contentsdebug-asserts that no pages are present — but a table that previously contained rows keeps its (row-empty) pages, so the reschema would panic in debug builds. NewPages::resetdrops residual pages first. This was unreachable for event tables (never any resident rows), so it surfaces only now.Behavior notes
RemoveTablestep; the message still names the table and explains the fix.scheduled_at/primary key columns move diffs toRemoveSchedule/AddSchedule, which remain unimplemented — such a migration still fails cleanly (pre-existing limitation, unchanged by this PR).API and ABI breaking changes
None on the module/client ABI.
AutoMigrateError::ReorderTableis removed andTableError::EventTableNotEmpty/PendingSchemaChange::ReschemaEventTableare renamed (crate-internal APIs).Expected complexity level and risk
2 — the mechanism reuses the event-table reschema path end to end; the new surface is the precheck plumbing, the replay condition, and the constraint carve-out, each covered by tests.
Testing
reorder_columns_of_empty_tableasserts the exact precheck and step lists (including sub-objectRemove*/Add*bracketing andDisconnectAllUsers).reorder_empty_table_succeeds(including the previously-contained-rows/residual-pages case; asserts new schema order, pk, exact pending-schema-change shape, and post-migration insert/read-back),reorder_nonempty_table_fails(clear error,pending_schema_changes() == [], old layout and data intact).replay_reordered_table_no_snapshot/replay_reordered_table_after_snapshot— a commitlog containing old-layout writes, the reorder, and new-layout writes replays correctly.spacetimedb-schema,spacetimedb-table,spacetimedb-datastore(--features test), andspacetimedb-enginepass locally.