tapdb: fix postgres sequence desyncs#2047
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses issues where Postgres sequences become desynchronized from their underlying table data due to legacy migrations that inserted rows with explicit IDs. By explicitly resetting these sequences and implementing a robust automated test to monitor sequence consistency in CI, the changes ensure database integrity for future migrations. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds migration 55 to synchronize Postgres sequences that were desynchronized by previous migrations and introduces a new test to verify sequence consistency. Feedback includes a critical fix for the migration script to prevent failures on empty tables, a reminder to update SQLite replacements if the SQL changes, and suggestions to improve the test logic and SQL safety.
Coverage Report for CI Build 24383514050Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage decreased (-0.005%) to 34.278%Details
Uncovered Changes
Coverage Regressions64 previously-covered lines in 9 files lost coverage.
Coverage Stats
💛 - Coveralls |
Migration 31 copied universe_leaves rows with explicit ids, leaving the BIGSERIAL sequence behind the actual data. New inserts then hit duplicate-key errors. Add migration 55 to reset the universe_leaves sequence via setval(). Also reset supply_commit_states and supply_commit_update_types (migration 40 inserted explicit ids into these enum tables); these are benign since no code path auto-increments into them, but fixing them keeps sequence state consistent. On SQLite the statements are replaced with no-ops.
Add TestSequenceConsistency, a Postgres-only test that verifies every BIGSERIAL sequence is consistent with its table's max ID after all migrations have run. This catches migrations that recreate tables and copy rows with explicit IDs without advancing the sequence.
Move the Postgres-to-SQLite replacement map into tapdb/sqlc/replacements.go so both tapdb/sqlite.go and cmd/merge-sql-schemas share a single source of truth.
There was a problem hiding this comment.
TBH I don't think we need a migration for this? Seems a bit over-engineered for a fix most likely no-one needs? Or do you think some users are suffering from this bug?
We can always keep the PR open and release in a patch if someone needs it someday
Yeah I think it's unlikely that there are other parties suffering from this bug, as it requires that they were 1) running prior to v0.6.0, and upgraded, 2) had substantial proof data, and 3) were running Postgres. But: anyone who does meet the criteria is probably fairly serious, and the bug means that their My intent with this is purely a just-in-case auto-fix, with the generic preventative test thrown in for good measure, but I think it's fine not to merge it. I definitely don't think we need it; if anyone needs the fix, we can just tell them to run the setval query manually, after all. |
Agreed. Good to have a proper fix and ready to be added in any patch release. Thanks! |
|
@GeorgeTsagk: review reminder |
4 participants
Fixes Postgres sequence desyncs caused by a couple of legacy migrations. One is material (in
universe_leaves, via migration 31) and the other two are benign (supply commitment-related enum values, via migration 40). Each desync'd sequence is simply set to its respective table's max id.Also adds a Postgres-only unit test that should catch future desyncs caused by migrations like this in CI; it simply checks that every BIGSERIAL sequence is consistent with its table's max id after all migrations have run.