Skip to content

feat(db): add generated availability_zone column and 9-column recommendations index - #808

Merged
cristim merged 2 commits into
mainfrom
feat/792-az-generated-column
Oct 12, 2026
Merged

cristim merged 2 commits into
mainfrom
feat/792-az-generated-column

Conversation

@cristim

@cristim cristim commented Oct 12, 2026

Copy link
Copy Markdown
Member

Part of #792 (PR2a of the zonal EC2 AZ work; PR1 is #807). Migration only, no Go store change.

What

Migration 000109 adds recommendations.availability_zone as GENERATED ALWAYS AS (COALESCE(payload->>'availability_zone', payload->'details'->>'availability_zone', '')) STORED and a new unique index recommendations_natural_key_az_idx on the existing 8 key columns plus availability_zone. The existing 8-column recommendations_natural_key_idx is kept, so the current ON CONFLICT upsert keeps working for old and new code. Down drops the new index and the column.

Existing rows are backfilled by the table rewrite from either payload shape (top-level field, or details.availability_zone for rows written before the top-level field existed); everything else becomes ''.

Known limitation (resolved in PR2c)

Two zonal rows differing only by AZ are still rejected by the kept 8-column index. The migration test documents this.

Lock impact

Adding a stored generated column rewrites the table, and the index build blocks writes. CREATE INDEX CONCURRENTLY is not possible because the runner wraps every migration in a transaction. The table is a regenerable cache repopulated every collection cycle, so this is acceptable.

Prod runbook

  1. Dispatch database-migration.yml for this PR (PR2a) before releasing PR2b.
  2. PR2b and PR2c release together; dispatch the PR2c migration immediately after PR2b is live.
    Dev auto-migrates on merge.

Verification

  • Integration test TestMigration_RecommendationsAvailabilityZone (up/down, backfill from legacy-details-only, both-shapes, top-level-only, regional and no-details rows, 8-column upsert still works, AZ-only pair still rejected by the 8-column index, index definition, rollback). Run locally against PostgreSQL 17 (the test body with a direct pool; testcontainers hangs on this machine), not through the container helper; CI runs the real one.
  • Mutations: dropping the details fallback fails the test; dropping availability_zone from the new index fails the index-definition assertion.

🤖 Generated with Claude Code

…ndations index

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 impact/many Affects most users effort/m Days type/bug Defect triaged Item has been triaged 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 3 billable files and costs up to $0.75.

  • 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 3 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 63 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: 1ec6d502-a86f-4280-9818-7f306996ad07

📥 Commits

Reviewing files that changed from the base of the PR and between 3fde4ef and c21611e.


📒 Files selected for processing (3)
  • internal/database/postgres/migrations/000109_recommendations_availability_zone.down.sql
  • internal/database/postgres/migrations/000109_recommendations_availability_zone.up.sql
  • internal/database/postgres/migrations/000109_recommendations_availability_zone_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 review (Opus 5.5, independent): CHANGES REQUESTED at 9ef9c32

Blocker: CI Lint Code fails on this SHA. Reproduced locally (golangci-lint 2.11.4, repo config):

internal/database/postgres/migrations/000109_recommendations_availability_zone_test.go:53:6: shadow: declaration of "err" shadows declaration at line 32 (govet)
		_, err := pool.Exec(ctx, insertRecForMigration109, resourceType, payload)

Fix: _, err = pool.Exec(...) on line 53. No other findings.

Migration review (clean):

  • Expression matches the stored payload: top-level RecommendationRecord.AvailabilityZone is json:"availability_zone,omitempty" (internal/config/types.go:539); Details is the plain json.Marshal of common.ComputeDetails (no custom MarshalJSON), whose field is json:"availability_zone,omitempty" (cloud-commitments-go pkg common/types.go:560). payload is JSONB, so ->>/-> are immutable and STORED is accepted (same pattern as account_key in 000030).
  • Old code keeps working: the only writer (store_postgres_recommendations.go:218) names an explicit column list without availability_zone; the only reader is SELECT payload. No SELECT *, COPY or views over recommendations. ON CONFLICT (8 cols) infers the kept 8-col index; a 9-col violation implies an 8-col one, so the new index cannot raise on an insert or on the DO UPDATE path.
  • Index build cannot fail on existing data (8-col unique is stricter). 000109 is free on main and in every open PR; branch is up to date with main.
  • Lock impact (table rewrite + index build in one tx on a regenerable cache) acceptable; dev auto-migrate failure is non-fatal (internal/server/app.go).

Local evidence (throwaway PostgreSQL 17.11 on 127.0.0.1, scratch DB, git archive of this SHA, go1.26.9):

  • TestMigration_RecommendationsAvailabilityZone body against the local pool (container setup swapped for pgxpool): PASS (108 -> 109 -> 108).
  • Extra psql probes in a rolled-back tx: up applied twice and down applied twice are idempotent; availability_zone: null with details AZ -> details AZ; details: null -> ''; store-shaped multi-row 8-col upsert that changes the AZ on conflict -> succeeds, column recomputed; both account_key and availability_zone are attgenerated = s.

Re-gate after the lint fix lands and CI is green.

@cristim

cristim commented Oct 12, 2026

Copy link
Copy Markdown
Member Author

Fixed govet shadow in the 000109 test at c21611e (golangci-lint with go1.26.9 and the integration tag: no issues). Waiting on CI.

@cristim

cristim commented Oct 12, 2026

Copy link
Copy Markdown
Member Author

Gate verdict (Opus 5.5, independent): APPROVED at c21611e

  • Delta re-gate. The only change since the reviewed SHA 9ef9c32 is the one-line test fix (_, err := -> _, err = at 000109_..._test.go:53). There is no main merge, and the migration SQL is unchanged. The earlier full review of the migration still applies (see the gate comment above).
  • Local check at this SHA: golangci-lint run ./internal/database/postgres/migrations/... -> 0 issues. The earlier local PostgreSQL 17.11 replay of the migration test (108->109->108) and the psql probes cover the unchanged SQL.
  • CI at this SHA: every check SUCCESS, including Lint Code and Integration Tests. The Integration Tests log shows --- PASS: TestMigration_RecommendationsAvailabilityZone (4.70s) (testcontainers). mergeStateStatus is CLEAN, and the labels mirror fix(purchase): AZ-scoped EC2 recs: the purchase guard, idempotency key and recommendations natural key ignore the AZ #792.
  • Mutation testing: none run, per the owner policy (migration only, not a money or purchase path).

Merging with --match-head-commit c21611e6f7c18dfdb0d09aede8ef25aafd14f640.

@cristim
cristim merged commit 631fb9c into main Oct 12, 2026
26 checks passed
@cristim
cristim deleted the feat/792-az-generated-column branch October 16, 2026 19:51
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