Skip to content

fix(db): drop the 8-column recommendations unique index so zonal EC2 AZ rows coexist (#792) - #811

Merged
cristim merged 1 commit into
mainfrom
fix/792-drop-8col-rec-index
Oct 12, 2026
Merged

cristim merged 1 commit into
mainfrom
fix/792-drop-8col-rec-index

Conversation

@cristim

@cristim cristim commented Oct 12, 2026

Copy link
Copy Markdown
Member

Closes part of #792 (PR2c, the last step of the zonal EC2 AZ work).

What

  • Migration 000110 drops the 8-column recommendations_natural_key_idx. The 9-column recommendations_natural_key_az_idx (000109) is now the only natural key, and the store already upserts on it (fix(config): upsert recommendations on the AZ-aware 9-column index #810). Two zonal EC2 rows that differ only by availability zone no longer abort the whole persistCollection write.
  • Down recreates the 8-column index exactly as 000043 defined it. It first deletes cache rows that differ only by AZ (keeping the newest collected_at), because the 8-column index cannot hold them. The table is a regenerable cache. Down is only safe together with a code rollback to the pre-fix(config): upsert recommendations on the AZ-aware 9-column index #810 8-column ON CONFLICT.

Production runbook (order matters)

  1. Dispatch database-migration.yml for 000109 first (if not already applied in prod).
  2. Release PR2b (fix(config): upsert recommendations on the AZ-aware 9-column index #810) and this PR together.
  3. Dispatch the 000110 migration immediately after PR2b is live in prod.
  4. Never apply 000110 before PR2b is deployed: the old code's 8-column ON CONFLICT needs that index, so every collection would fail.

Dev auto-migrates on merge.

Merge readiness

Not safe to merge until PR2b (#810, main commit 26470c3) is deployed to dev. At the time of writing the deploy workflows for that SHA (GCP, Azure, AWS) were still in progress, so dev still ran the old code. Merging earlier would auto-migrate dev under the 8-column upserter. Check the three deploy runs for 26470c3 are green first.

Tests (integration, against a throwaway local PostgreSQL 17; testcontainers hangs on this machine, so CI's testcontainers run is the authoritative one)

  • TestMigration_DropRecommendations8ColNaturalKey: index dropped, 9-column kept, AZ pair coexists, same-AZ and regional duplicates still rejected, down collapses AZ duplicates and recreates the 8-column index with its original definition, re-up works.
  • TestPostgresStore_UpsertRecommendations_MultiAZBatch: two zonal rows in ONE batch upsert with no error and both persist; same-AZ re-upsert updates in place; regional row unaffected; ReplaceRecommendations stores the pair.
  • TestPersistCollectionMultiAZInOneBatch: a 500-row collection with two zonal recs on the same 8-tuple persists fully and records no write error.
  • Pre-fix proof: with the 000110 files removed, the store test fails ("8-column index must be gone") and the scheduler test fails with duplicate key ... recommendations_natural_key_idx and 0 rows stored.
  • golangci-lint (go1.26.9) clean on the touched packages.

Real-scenario evidence is a local Postgres 17 running the real migration chain and the real store path; no mocks. Not exercised: an actual AWS collection.

🤖 Generated with Claude Code

…AZ rows coexist (#792)

Migration 000110 drops recommendations_natural_key_idx; the store already upserts on the 9-column AZ-aware index (#810). Down recreates the 8-column index after collapsing AZ-only duplicates.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@cristim cristim added priority/p2 Backlog-worthy severity/medium Moderate harm urgency/this-sprint Within the current sprint triaged Item has been triaged impact/many Affects most users effort/m Days type/bug Defect labels Oct 12, 2026
@coderabbitai

coderabbitai Bot commented Oct 12, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

  • Run on-demand review

This review includes 5 billable files and costs up to $1.25.

  • Ask an admin to make reviews automatic

Open in CodeRabbit

Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing.

Or wait 28 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available. Your 61 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Essentials
  • Run ID: 14e50671-47d8-4e60-9962-399d199e4ecd

📥 Commits

Reviewing files that changed from the base of the PR and between 26470c3 and 6f48c67.


📒 Files selected for processing (5)
  • internal/config/store_postgres_recommendations_test.go
  • internal/database/postgres/migrations/000110_drop_recommendations_8col_natural_key.down.sql
  • internal/database/postgres/migrations/000110_drop_recommendations_8col_natural_key.up.sql
  • internal/database/postgres/migrations/000110_drop_recommendations_8col_natural_key_test.go
  • internal/scheduler/scheduler_global_filters_integration_test.go

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@cristim

cristim commented Oct 12, 2026

Copy link
Copy Markdown
Member Author

Gate verdict: APPROVE at 6f48c67

Independent review (Opus 5.5, claude-opus-5-5), merge gate for #792 PR2c.

Review

  • Up: DROP INDEX IF EXISTS recommendations_natural_key_idx; name and 8 columns match 000043 (account_key, provider, service, region, resource_type, engine, term, payment_option). 000110 follows 000109 on main; no open PR adds another 0001xx migration.
  • Down: before 000110 the 8-col key was unique, and after it the 9-col index stays unique, so any 8-col duplicate differs only by AZ. The ROW_NUMBER() partition on the 8 columns, ORDER BY collected_at DESC, id DESC, keeps the newest row and deletes only those AZ-only duplicates. All 8 key columns are NOT NULL (account_key is generated via COALESCE), so NULL grouping cannot over-delete. Index recreated with the 000043 definition. Safe only together with a code rollback to the 8-col ON CONFLICT, as the comment says.
  • Dependents: git grep at head finds only one ON CONFLICT on recommendations: store_postgres_recommendations.go:230, which targets the 9 columns (fix(config): upsert recommendations on the AZ-aware 9-column index #810). No 8-col arbiter is left in code, scripts, seed data or later migrations. The one 8-col upsert, in 000109_..._test.go:101, migrates only to 109, so it is unaffected.
  • No findings. CodeRabbit was rate-limited and posted no review; it is optional under the gate.

Local verification (throwaway PostgreSQL 17.11 on 127.0.0.1, git archive of the head, go1.26.9, -tags integration; testhelpers pointed at the local server because testcontainers hangs on this machine)

  • Head: PASS for TestMigration_DropRecommendations8ColNaturalKey, TestMigration_RecommendationsAvailabilityZone, TestPostgresStore_UpsertRecommendations_* (including _MultiAZBatch), TestPersistCollectionMultiAZInOneBatch and TestGlobalFiltersPersistedAmbientAndRegistered.
  • Baseline with the 000110 files removed and the tests kept: TestPersistCollectionMultiAZInOneBatch FAILs with SQLSTATE 23505 on recommendations_natural_key_idx and 0 of 500 rows stored. _MultiAZBatch, with its index precondition removed, FAILs on every subtest with failed to insert recommendations batch: ... 23505 ... recommendations_natural_key_idx.
  • CI Integration Tests (real Postgres container, run 38285628930) ran and passed all four new or affected tests.

Rollout

  • PR2b (fix(config): upsert recommendations on the AZ-aware 9-column index #810, 26470c3) is live on dev. The AWS Lambda (38285099249), GCP Cloud Run (38285099332) and Azure Container Apps (38285099258) deploys each passed Build & Deploy and Test Deployment, and the image tag is 26470c3. No newer main commit exists.
  • Dev rollout window: every running instance has the 9-col ON CONFLICT, so dropping the 8-col index while instances overlap during the auto-migrate cannot break them. Acceptable.
  • Prod must follow the runbook in the PR body: 000109 first, release PR2b and PR2c together, then dispatch 000110 only after PR2b is live, never before.

State at the SHA: all checks SUCCESS, mergeStateStatus CLEAN, labels mirror #792.

@cristim
cristim merged commit d2afc4e into main Oct 12, 2026
25 checks passed
@cristim
cristim deleted the fix/792-drop-8col-rec-index branch October 16, 2026 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/m Days impact/many Affects most users priority/p2 Backlog-worthy severity/medium Moderate harm triaged Item has been triaged type/bug Defect urgency/this-sprint Within the current sprint

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant