Remove non-main migration line rows when migrating down past version 1 - #1378
Merged
Merged
Conversation
bgentry
force-pushed
the
bg/migrate-line-down-v1
branch
from
September 24, 2026 17:35
40d5c06 to
e35702c
Compare
brandur
approved these changes
Sep 29, 2026
brandur
left a comment
brandur
left a comment
Contributor
There was a problem hiding this comment.
This seems right. Going to bring it in.
brandur
marked this pull request as ready for review
September 29, 2026 21:22
The migrator treats version 1 as special when migrating down: the main line's version 1 drops `river_migration`, so there's no row left to delete. That special case applies to every line, though, and a non-main line's version 1 doesn't touch `river_migration`. Migrating such a line down through version 1 leaves its version 1 row in place, and with `MigrateTx` it leaves the rows for every version removed in that run. A later up migration then sees version 1 as still applied and skips it, and another down migration reruns version 1's down SQL. Scope both version 1 special cases (the per-version delete and the batch delete used within an outer transaction) to the main line, so other lines delete their rows like any other version. Extend the alternate line tests to check that migrating fully down removes all of the line's rows and that migrating back up reapplies version 1, add a test that stepping down from version 1 removes its row and leaves nothing more to migrate, and cover the same round trip through `MigrateTx`.
brandur
force-pushed
the
bg/migrate-line-down-v1
branch
from
September 29, 2026 21:24
e35702c to
f57955a
Compare
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
2 participants
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.
When a migration line other than
mainis migrated down through its version 1,rivermigrateleaves that version's row inriver_migration. Only the main line's version 1 drops the table, so the "nothing left to delete" shortcut is wrong for other lines. WithMigrateTxthe same shortcut leaves rows for every version removed in the run. Afterwards, a later up migration skips the line's version 1, and a later down migration runs its down SQL a second time.This limits both version 1 shortcuts to the main line, so other lines delete their rows like any other version. New tests cover a full down-and-up round trip on an alternate line, with and without an outer transaction, and stepping down from version 1 one step at a time.