tapdb: fix multiple issues around migration file handling #1089
Conversation
This file contains 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
This commit adds a check script to our CI that makes sure there are no "holes" in the numbering of our migration version files. The script would also detect duplicate numbers in the migration prefixes. This is to prevent mistakes from cherry-picking commits onto branches that contain migration files.
Pull Request Test Coverage Report for Build 10406480087Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
guggero
force-pushed
the
fix-migration-14
branch
from
b38865f to
1e3feb3
Compare
Due to a git branch cherry-pick mishap, the migration number 14 wasn't included in a released version and therefore wasn't applied on some systems. Because the tables in that migration file were only being used with a recently released version (v0.4.0/v0.4.1), this wasn't noticed for a long time. We fix this by re-applying the same migration as number 23, slightly modified so that it's a no-op if the tables and entries already exist on a system.
guggero
force-pushed
the
fix-migration-14
branch
from
1e3feb3 to
1826364
Compare
Our SQL scripts on disk were neither SQLite nor Postgres compatible, we used a version suited for sqlc to generate the correct data types we wanted, then did some type replacements in-memory when actually applying the migration files. This is somewhat dangerous as it will lead to situations where if a user applies a migration file manually, they'll end up with table primary keys that ARE NOT auto incrementing. To fix this issue, we make sure that we always write fully SQLite compatible SQL code and only apply fixes for the sqlc code generation directly before generating the code.
guggero
force-pushed
the
fix-migration-14
branch
from
1826364 to
17ae210
Compare
ffranr
approved these changes
Aug 15, 2024
|
Another fee estimation itest here: I will re-run the job. |
Maybe we need to enable the docker image cache, since we pull a new Postgres image every time we run the CI with docker... |
jharveyb
approved these changes
Aug 15, 2024
|
Going to verify the postgres itest suceeds locally before merging. I'm also going to build a |
Applied this to my node without problems and also ran the full postgres itest locally. |
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.
Fixes #1071.
This PR is the result after almost two days of debugging #1071.
The TL;DR is: due to a git branch cherry-pick mishap, the migration number 14 wasn't included in the
v0.3.3release version. Some users then applied the migration directly as it was in the source directory, which lead to them not having properly working auto incrementing primary keys.The first commit in this PR adds a CI check that verifies migration numbers in the file names are continuous.
The second commit re-applies the migration 14 (somewhat modified) as migration 23.
The last commit then fixes the workaround with the primary key type replacements we used. See comment in
scripts/gen_sqlc_docker.shif you want to understand the full problem and solution.