Repository navigation
fix(migrations): repair partially-applied 058-067 schema (idempotent re-assert) - #1127
Merged
cristim merged 2 commits intoJun 9, 2026
Merged
Conversation
…re-assert) A prior emergency Force(67) on a dev/QA database marked migrations 058-067 as applied without running their SQL, leaving genuinely-unapplied schema changes missing. The most visible symptom is live 500s on planned/pending/stuck purchase-execution queries: "column idempotency_key does not exist" (purchase_executions.idempotency_key from 066). groups.system_managed (059) and other 058-067 schema are missing too. The 058-067 range cannot simply be re-run: migration 059 has a RAISE EXCEPTION seed-guard that conflicts with the Purchaser group already relocated by 064. Add migration 000074, which re-asserts ONLY the schema from 058-067 in fully idempotent form so any partially-migrated (or fully-migrated) DB converges to the correct schema. It intentionally omits the conflicting data seeds (059 Purchaser INSERT + admin backfill, 060 universal-plans DELETE, 064 relocate); those rows already exist or were deliberately relocated on any affected DB. Schema re-asserted, by source migration: - 058: purchase_executions.executed_by_user_id, .executed_at, .pre_approval_skip_reason; index idx_executions_direct_execute - 059: groups.system_managed - 063: purchase_history.monthly_cost DROP NOT NULL - 065: function check_min_one_admin(); constraint triggers trg_min_one_admin_delete, trg_min_one_admin_update - 066: purchase_executions.idempotency_key - 067: savings_snapshots.account_id widened to VARCHAR(255); total_usage / coverage_percentage made NULLABLE (DROP NOT NULL + DROP DEFAULT); materialized views monthly_savings_summary, daily_savings_trend, provider_savings_summary recreated carrying cloud_account_id (+ unique indexes); function drop_old_savings_partitions() Ordering differs from source 067 in one place: the materialized views are dropped BEFORE the account_id type widening. On the corrupted DB the views still exist from migration 000003 selecting account_id, which blocks ALTER COLUMN account_id TYPE. Dropping the dependent views first lets the repair converge from the view-present state. The migration is fully transactional (no CONCURRENTLY), so golang-migrate applies it atomically, and it is idempotent (re-applying is a clean no-op). The .down.sql is a documented no-op: this repair does not own the objects (the source migrations do), so dropping them on rollback would corrupt a correctly-migrated DB. Verified against a Postgres 16 container: reproduced the corrupted baseline (001-057 applied, 058-067 skipped, 070-073 applied), confirmed all listed objects were missing, applied 074, and confirmed every object converged; re-applied 074 and confirmed it is a clean no-op; confirmed it runs inside a single transaction. go build and migration unit tests green, gofmt clean.
Contributor
|
Warning Review limit reached
More reviews will be available in 16 minutes and 18 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds migration 000074 to repair databases where migrations 058–067 were incorrectly marked applied without fully executing. The migration atomically re-asserts missing schema across purchase execution audit columns, admin enforcement triggers, and analytics snapshot definitions, with a no-op rollback script. ChangesMigration repair for partial application
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
cristim
added
type/bug
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@internal/database/postgres/migrations/000074_repair_partial_migration_058_067.up.sql`:
- Around line 183-271: The CREATE MATERIALIZED VIEW statements for
monthly_savings_summary, daily_savings_trend and provider_savings_summary
currently populate immediately and then the subsequent REFRESH MATERIALIZED VIEW
calls re-run the same work; fix by either adding WITH NO DATA to each CREATE
MATERIALIZED VIEW (monthly_savings_summary, daily_savings_trend,
provider_savings_summary) so they are created empty and the later REFRESH is the
single populate step, or remove the three REFRESH MATERIALIZED VIEW lines
entirely so creation does the single populate—choose one approach and apply it
consistently to those three view definitions and/or the REFRESH calls.
- Around line 283-303: The drop_old_savings_partitions function currently scans
public.pg_tables with tablename LIKE 'savings_snapshots_%' which can match
non-partition tables; change the partition enumeration to query
pg_inherits/pg_class to list only actual child partitions of the parent table
savings_snapshots (join pg_inherits.inhrelid -> pg_class.relname, filter parent
relname = 'savings_snapshots' and exclude the default partition name), then
parse the YYYY_MM from each child partition name as before and only EXECUTE DROP
for those child partitions older than cutoff_date; update references in the
function to use the pg_inherits-derived partition_record.tablename (or relname)
rather than pg_tables so non-child tables like savings_snapshots_backup_* are
never considered for drop.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e0c2714e-6f7a-4a68-a836-942891df4763
📒 Files selected for processing (2)
internal/database/postgres/migrations/000074_repair_partial_migration_058_067.down.sqlinternal/database/postgres/migrations/000074_repair_partial_migration_058_067.up.sql
Two CodeRabbit findings on the idempotent 058-067 repair migration: - Materialized views (monthly_savings_summary, daily_savings_trend, provider_savings_summary) were created without WITH NO DATA, so each populated immediately and the subsequent REFRESH re-ran the same aggregation, doubling work and lock time on savings_snapshots. Create the views WITH NO DATA so the single REFRESH is the only populate step. - drop_old_savings_partitions enumerated partitions via pg_tables with tablename LIKE 'savings_snapshots_%', where the LIKE wildcard '_' could match unrelated tables (e.g. savings_snapshots_backup_2024_01) and risk dropping non-partition data. Enumerate real child partitions from the catalog (pg_inherits) scoped to the savings_snapshots parent instead. Migration stays fully idempotent, transactional, and CONCURRENTLY-free; no 059 data seed is reintroduced. go build ./... and go test ./internal/database/... pass.
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
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
1 participant
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.
Root cause
A prior emergency
Force(67)on a dev/QA database marked migrations 058-067 asapplied without running their SQL. Several genuinely-unapplied schema
changes from that range are therefore missing in the affected DB. The most
visible symptom is live 500s on planned/pending/stuck purchase-execution
queries:
(
purchase_executions.idempotency_key, added by migration000066). Confirmedalso missing:
groups.system_managed(000059), and the rest of the 058-067schema (058 audit columns/index, 063 nullable
monthly_cost, 065 min-one-admintriggers, 067 analytics-snapshot corrections).
The 058-067 range cannot simply be re-run: migration
000059carries aRAISE EXCEPTIONseed-guard that conflicts with thePurchasergroup alreadyrelocated by
000064, so re-applying it aborts.Fix
New migration
000074_repair_partial_migration_058_067re-asserts only theschema from 058-067 in fully idempotent form, so any partially-migrated
(or fully-migrated) DB self-heals to the correct schema on the next deploy. It
intentionally omits the conflicting data seeds (059 Purchaser INSERT + admin
backfill, 060 universal-plans DELETE, 064 relocate); those rows already exist or
were deliberately relocated on any affected DB.
Schema re-asserted (by source migration)
purchase_executions.executed_by_user_id,.executed_at,.pre_approval_skip_reasonADD COLUMN IF NOT EXISTSidx_executions_direct_executeCREATE INDEX IF NOT EXISTSgroups.system_managedADD COLUMN IF NOT EXISTSpurchase_history.monthly_costnullableDROP NOT NULL(no-op when already applied)check_min_one_admin()CREATE OR REPLACE FUNCTIONtrg_min_one_admin_delete,trg_min_one_admin_updateDROP TRIGGER IF EXISTS+CREATE TRIGGERpurchase_executions.idempotency_keyADD COLUMN IF NOT EXISTSsavings_snapshots.account_idwidened toVARCHAR(255)DOblocksavings_snapshots.total_usage/.coverage_percentagenullableDROP NOT NULL+DROP DEFAULTmonthly_savings_summary,daily_savings_trend,provider_savings_summary(+ unique indexes)DROP MATERIALIZED VIEW IF EXISTS ... CASCADEthenCREATEdrop_old_savings_partitions()CREATE OR REPLACE FUNCTIONOne deliberate ordering change vs source 067
The materialized views are dropped before the
account_idtype widening.On the corrupted DB the views still exist from migration
000003selectingsavings_snapshots.account_id, which blocksALTER COLUMN account_id TYPEwith
cannot alter type of a column used by a view or rule. Dropping thedependent views first lets the repair converge from the view-present state, then
recreates them with the corrected
cloud_account_id-carrying definitions.Transactional + idempotent
CONCURRENTLYanywhere, so golang-migrate applies the whole file in asingle transaction (atomic; partial application impossible).
already-correct DBs and safe to re-run.
.down.sqlis a documented no-op: this repair does not own the objects(the source migrations do), so dropping them on rollback would corrupt a
correctly-migrated DB.
Verification
Against a Postgres 16 container:
001-057, skipped 058-067,applied
070-073. Confirmed every listed object was missing(
idempotency_keyabsent,system_managedabsent,account_id= 20,monthly_costNOT NULL, 0 admin triggers, old views).000074-> every object converged (idempotency_keypresent,account_id= 255, both triggers + function present, all three matviewsrecreated carrying
cloud_account_idwith their unique indexes).000074-> clean no-op, state unchanged.psql -1(single transaction) -> succeeds atomically.go build ./...succeeds, the migration unit-test suite passes (175 tests),gofmtclean.🤖 Generated with claude-flow
Summary by CodeRabbit