Skip to content

Auto-migrate: Allow adding new variants at the tail#2874

Merged
Centril merged 28 commits into
masterfrom
centril/auto-migrate-sum-prefix
Jun 27, 2025
Merged

Auto-migrate: Allow adding new variants at the tail#2874
Centril merged 28 commits into
masterfrom
centril/auto-migrate-sum-prefix

Conversation

@Centril
Copy link
Copy Markdown
Contributor

@Centril Centril commented Jun 16, 2025

Description of Changes

To the auto-migrate machinery, detection of the case where a sum type is extended with additional variants and where the sum type as a whole does not grow in size. This then results in a new datastore call alter_table_row_type which independently checks for "soundness" of using the new row type for the existing rows. This call, being a schema altering one, is handled, in terms of transactionality, the same way that e.g., create_index is, by adding a pending schema change.

API and ABI breaking changes

None

Expected complexity level and risk

3 - this is fairly sensitive logic that is critical to get right so that we don't cause UB and problems down the line.

Testing

  • The datastore functionality is independently tested to see that it works and for transactionality.
  • The datastore API alter_table_access was untested and is now tested also in terms of transactionality.
  • The auto-migration logic is tested comprehensively both for success and failure.
  • A smoketest is amended to have an integration test as well.

Comment thread crates/core/src/db/datastore/locking_tx_datastore/mut_tx.rs Outdated
Comment thread crates/table/src/table.rs Outdated
@bfops bfops added the release-any To be landed in any release window label Jun 23, 2025
@Centril Centril force-pushed the centril/auto-migrate-sum-prefix branch from c43dff2 to afac310 Compare June 25, 2025 18:15
@Centril Centril changed the title [DRAFT] Auto-migrate: allow adding new variants at the tail Auto-migrate: Allow adding new variants at the tail Jun 25, 2025
@Centril Centril requested a review from gefjon June 25, 2025 19:37
Copy link
Copy Markdown
Contributor

@gefjon gefjon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comment-ey nits, and a significant logic concern: I think that renaming enum variants or field names should require a manual migration. There's no technical reason why we have to do this, but I think that using ordering as a source of truth is a surprising and footgun-ey behavior.

Comment thread crates/core/src/db/datastore/locking_tx_datastore/tx_state.rs
Comment thread crates/core/src/db/datastore/locking_tx_datastore/mut_tx.rs Outdated
Comment thread crates/core/src/db/datastore/locking_tx_datastore/mut_tx.rs
Comment thread crates/core/src/db/datastore/locking_tx_datastore/committed_state.rs Outdated
Comment thread crates/schema/src/auto_migrate.rs Outdated
Comment thread crates/schema/src/auto_migrate.rs Outdated
Comment thread crates/schema/src/auto_migrate.rs
@Centril Centril requested a review from gefjon June 27, 2025 15:14
Copy link
Copy Markdown
Contributor

@gefjon gefjon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beautiful, thanks!

@Centril Centril added this pull request to the merge queue Jun 27, 2025
Merged via the queue into master with commit 0c36351 Jun 27, 2025
18 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-any To be landed in any release window

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants