From 6de9d9242c925bbe8aa2758f8e483c3c53eb7bc7 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 10 Jun 2026 12:29:57 -0700 Subject: [PATCH 1/6] fix(analytics): nested SUM-then-AVG rollup in breakdowns and monthly view Snapshot rows are run-rates written at (account, provider, service, region, commitment_type, timestamp) grain, so a breakdown bucket holds several rows sharing the SAME timestamp (one per region / commitment type / account). QueryByProvider, QueryByService and the monthly_savings_summary materialized view used a flat AVG across all rows in the bucket, which returns the mean per-row run-rate instead of the bucket's total: $100/mo in each of 5 regions reported $100 instead of $500, understating savings for any multi-region estate. Apply the H5 nested shape that daily_savings_trend and provider_savings_summary already use: the inner query SUMs the per-row run-rates into the bucket's instant total at each collection timestamp, the outer query AVGs those instant totals over time so results stay invariant to collection frequency. - QueryByProvider / QueryByService: rewrite to the nested rollup. - Migration 000076: recreate monthly_savings_summary with the nested rollup (snapshot_count keeps raw-row semantics, cast back to BIGINT); down restores the 000067/000074 flat-AVG definition. - Regression test (integration tag) replicating the failing scenario: two regions plus two commitment types sharing a timestamp; asserts buckets report the per-timestamp sum (325) not the row mean (108.33) across both Go queries and the view, plus a down/up round-trip. All subtests fail against the pre-fix code. - Update pgxmock SQL-shape expectations to assert the nested query. Closes #1151 --- internal/analytics/postgres_analytics.go | 42 +++-- internal/analytics/postgres_analytics_test.go | 16 +- ...076_monthly_summary_nested_rollup.down.sql | 25 +++ ...00076_monthly_summary_nested_rollup.up.sql | 63 ++++++++ ...0076_monthly_summary_nested_rollup_test.go | 149 ++++++++++++++++++ 5 files changed, 277 insertions(+), 18 deletions(-) create mode 100644 internal/database/postgres/migrations/000076_monthly_summary_nested_rollup.down.sql create mode 100644 internal/database/postgres/migrations/000076_monthly_summary_nested_rollup.up.sql create mode 100644 internal/database/postgres/migrations/000076_monthly_summary_nested_rollup_test.go diff --git a/internal/analytics/postgres_analytics.go b/internal/analytics/postgres_analytics.go index 4b9968c6a..6af6cbbc8 100644 --- a/internal/analytics/postgres_analytics.go +++ b/internal/analytics/postgres_analytics.go @@ -345,13 +345,26 @@ func (s *PostgresAnalyticsStore) QueryMonthlyTotals(ctx context.Context, account func (s *PostgresAnalyticsStore) QueryByProvider(ctx context.Context, accountUUIDs []string, accountExternalIDsByProvider map[string][]string, startDate, endDate time.Time) ([]ProviderBreakdown, error) { accountClause, args := accountFilterClause(accountUUIDs, accountExternalIDsByProvider, []any{startDate, endDate}) + // Snapshot rows are run-rates written at (account, provider, service, + // region, commitment_type, timestamp) grain, so a (provider, service) + // bucket spans several rows at the SAME timestamp. Per H5 the inner query + // sums those rows into the bucket's instant total at each collection + // timestamp and the outer query averages the instant totals over time; a + // flat AVG would report the mean per-row run-rate instead of the bucket + // total (COR-02). // #nosec G201 — accountClause uses only internally-built placeholders. query := ` - SELECT provider, service, AVG(total_savings) as total_savings, AVG(coverage_percentage) as avg_coverage - FROM savings_snapshots - WHERE timestamp >= $1 - AND timestamp <= $2 - AND ` + accountClause + ` + SELECT provider, service, AVG(ts_savings) as total_savings, AVG(ts_coverage) as avg_coverage + FROM ( + SELECT provider, service, timestamp, + SUM(total_savings) as ts_savings, + AVG(coverage_percentage) as ts_coverage + FROM savings_snapshots + WHERE timestamp >= $1 + AND timestamp <= $2 + AND ` + accountClause + ` + GROUP BY provider, service, timestamp + ) per_ts GROUP BY provider, service ORDER BY total_savings DESC ` @@ -392,13 +405,22 @@ func (s *PostgresAnalyticsStore) QueryByService(ctx context.Context, accountUUID providerClause = fmt.Sprintf(" AND provider = $%d", len(args)) } + // Same H5 nested rollup as QueryByProvider: a (service, region) bucket + // spans accounts and commitment types at the same timestamp, so SUM the + // rows per timestamp first, then AVG the instant totals over time (COR-02). // #nosec G201 — accountClause / providerClause use only internally-built placeholders. query := fmt.Sprintf(` - SELECT service, region, AVG(total_savings) as total_savings, AVG(coverage_percentage) as avg_coverage - FROM savings_snapshots - WHERE timestamp >= $1 - AND timestamp <= $2 - AND %s%s + SELECT service, region, AVG(ts_savings) as total_savings, AVG(ts_coverage) as avg_coverage + FROM ( + SELECT service, region, timestamp, + SUM(total_savings) as ts_savings, + AVG(coverage_percentage) as ts_coverage + FROM savings_snapshots + WHERE timestamp >= $1 + AND timestamp <= $2 + AND %s%s + GROUP BY service, region, timestamp + ) per_ts GROUP BY service, region ORDER BY total_savings DESC `, accountClause, providerClause) diff --git a/internal/analytics/postgres_analytics_test.go b/internal/analytics/postgres_analytics_test.go index e2f651fdc..f55daf3a3 100644 --- a/internal/analytics/postgres_analytics_test.go +++ b/internal/analytics/postgres_analytics_test.go @@ -580,7 +580,7 @@ func TestQueryByProvider(t *testing.T) { AddRow("aws", "rds", 2500.0, f64ptr(85.0)). AddRow("aws", "elasticache", 1200.0, f64ptr(75.0)) - mock.ExpectQuery(`SELECT provider, service, AVG\(total_savings\) as total_savings`). + mock.ExpectQuery(`(?s)SELECT provider, service, AVG\(ts_savings\) as total_savings.*SUM\(total_savings\) as ts_savings.*GROUP BY provider, service, timestamp`). WithArgs(startDate, now, []string{"account-123"}). WillReturnRows(rows) @@ -603,7 +603,7 @@ func TestQueryByProvider(t *testing.T) { now := time.Now().UTC() startDate := now.Add(-30 * 24 * time.Hour) - mock.ExpectQuery(`SELECT provider, service, AVG\(total_savings\) as total_savings`). + mock.ExpectQuery(`(?s)SELECT provider, service, AVG\(ts_savings\) as total_savings.*SUM\(total_savings\) as ts_savings.*GROUP BY provider, service, timestamp`). WithArgs(startDate, now, []string{"account-123"}). WillReturnError(errors.New("database error")) @@ -632,7 +632,7 @@ func TestQueryByService(t *testing.T) { AddRow("rds", "us-east-1", 1800.0, f64ptr(90.0)). AddRow("rds", "us-west-2", 700.0, f64ptr(75.0)) - mock.ExpectQuery(`SELECT service, region, AVG\(total_savings\) as total_savings`). + mock.ExpectQuery(`(?s)SELECT service, region, AVG\(ts_savings\) as total_savings.*SUM\(total_savings\) as ts_savings.*GROUP BY service, region, timestamp`). WithArgs(startDate, now, []string{"account-123"}, "aws"). WillReturnRows(rows) @@ -655,7 +655,7 @@ func TestQueryByService(t *testing.T) { now := time.Now().UTC() startDate := now.Add(-30 * 24 * time.Hour) - mock.ExpectQuery(`SELECT service, region, AVG\(total_savings\) as total_savings`). + mock.ExpectQuery(`(?s)SELECT service, region, AVG\(ts_savings\) as total_savings.*SUM\(total_savings\) as ts_savings.*GROUP BY service, region, timestamp`). WithArgs(startDate, now, []string{"account-123"}, "aws"). WillReturnError(errors.New("database error")) @@ -1141,7 +1141,7 @@ func TestQueryByProviderRowScanError(t *testing.T) { "provider", // Missing other columns }).AddRow("aws").RowError(0, errors.New("scan error")) - mock.ExpectQuery(`SELECT provider, service, AVG\(total_savings\) as total_savings`). + mock.ExpectQuery(`(?s)SELECT provider, service, AVG\(ts_savings\) as total_savings.*SUM\(total_savings\) as ts_savings.*GROUP BY provider, service, timestamp`). WithArgs(startDate, now, []string{"account-123"}). WillReturnRows(rows) @@ -1167,7 +1167,7 @@ func TestQueryByServiceRowScanError(t *testing.T) { "service", // Missing other columns }).AddRow("rds").RowError(0, errors.New("scan error")) - mock.ExpectQuery(`SELECT service, region, AVG\(total_savings\) as total_savings`). + mock.ExpectQuery(`(?s)SELECT service, region, AVG\(ts_savings\) as total_savings.*SUM\(total_savings\) as ts_savings.*GROUP BY service, region, timestamp`). WithArgs(startDate, now, []string{"account-123"}, "aws"). WillReturnRows(rows) @@ -1258,7 +1258,7 @@ func TestQueryByProviderRowsErr(t *testing.T) { "provider", "service", "total_savings", "avg_coverage", }).AddRow("aws", "rds", 2500.0, f64ptr(85.0)) - mock.ExpectQuery(`SELECT provider, service, AVG\(total_savings\) as total_savings`). + mock.ExpectQuery(`(?s)SELECT provider, service, AVG\(ts_savings\) as total_savings.*SUM\(total_savings\) as ts_savings.*GROUP BY provider, service, timestamp`). WithArgs(startDate, now, []string{"account-123"}). WillReturnRows(rows) @@ -1285,7 +1285,7 @@ func TestQueryByServiceRowsErr(t *testing.T) { "service", "region", "total_savings", "avg_coverage", }).AddRow("rds", "us-east-1", 1800.0, f64ptr(90.0)) - mock.ExpectQuery(`SELECT service, region, AVG\(total_savings\) as total_savings`). + mock.ExpectQuery(`(?s)SELECT service, region, AVG\(ts_savings\) as total_savings.*SUM\(total_savings\) as ts_savings.*GROUP BY service, region, timestamp`). WithArgs(startDate, now, []string{"account-123"}, "aws"). WillReturnRows(rows) diff --git a/internal/database/postgres/migrations/000076_monthly_summary_nested_rollup.down.sql b/internal/database/postgres/migrations/000076_monthly_summary_nested_rollup.down.sql new file mode 100644 index 000000000..2e7a0a556 --- /dev/null +++ b/internal/database/postgres/migrations/000076_monthly_summary_nested_rollup.down.sql @@ -0,0 +1,25 @@ +-- 000076 down: restore the flat-AVG monthly_savings_summary definition from +-- 000067/000074 (which understates multi-row buckets, per COR-02). +DROP MATERIALIZED VIEW IF EXISTS monthly_savings_summary CASCADE; + +CREATE MATERIALIZED VIEW monthly_savings_summary AS +SELECT + DATE_TRUNC('month', timestamp) as month, + account_id, + cloud_account_id, + provider, + service, + AVG(total_savings) as total_savings, + AVG(coverage_percentage) as avg_coverage, + AVG(total_commitment) as total_commitment, + AVG(total_usage) as total_usage, + COUNT(*) as snapshot_count, + MAX(timestamp) as last_updated +FROM savings_snapshots +GROUP BY DATE_TRUNC('month', timestamp), account_id, cloud_account_id, provider, service; + +CREATE UNIQUE INDEX idx_monthly_savings_summary_unique + ON monthly_savings_summary( + month, account_id, + COALESCE(cloud_account_id, '00000000-0000-0000-0000-000000000000'::uuid), + provider, service); diff --git a/internal/database/postgres/migrations/000076_monthly_summary_nested_rollup.up.sql b/internal/database/postgres/migrations/000076_monthly_summary_nested_rollup.up.sql new file mode 100644 index 000000000..31a81132f --- /dev/null +++ b/internal/database/postgres/migrations/000076_monthly_summary_nested_rollup.up.sql @@ -0,0 +1,63 @@ +-- 000076: monthly_savings_summary nested SUM-then-AVG rollup (COR-02). +-- +-- Snapshot rows are run-rates written at (account, provider, service, region, +-- commitment_type, timestamp) grain, so a (month, account, provider, service) +-- bucket contains several rows sharing the SAME timestamp (one per region / +-- commitment type). The 000067/000074 definition used a flat AVG across all +-- rows in the bucket, which returns the mean per-row run-rate instead of the +-- bucket's total: $100/mo in each of 5 regions reported total_savings=$100 +-- instead of $500. +-- +-- Apply the same H5 shape daily_savings_trend and provider_savings_summary +-- already use: the inner query sums the per-row run-rates into the bucket's +-- instant total at each collection timestamp, the outer query averages those +-- instant totals over the month so the result stays invariant to the +-- collection frequency. snapshot_count keeps its raw-row semantics via +-- SUM(ts_row_count) (cast back to BIGINT because SUM(bigint) yields NUMERIC). +-- +-- DROP + CREATE because a materialized view's defining query cannot be +-- altered in place. The unique index is recreated for the CONCURRENTLY +-- refresh path, and the view is created populated (no WITH NO DATA) so +-- refresh_savings_materialized_views() can keep using CONCURRENTLY. +DROP MATERIALIZED VIEW IF EXISTS monthly_savings_summary CASCADE; + +CREATE MATERIALIZED VIEW monthly_savings_summary AS +SELECT + month, + account_id, + cloud_account_id, + provider, + service, + AVG(ts_savings) as total_savings, + AVG(ts_coverage) as avg_coverage, + AVG(ts_commitment) as total_commitment, + AVG(ts_usage) as total_usage, + SUM(ts_row_count)::BIGINT as snapshot_count, + MAX(timestamp) as last_updated +FROM ( + SELECT + DATE_TRUNC('month', timestamp) as month, + timestamp, + account_id, + cloud_account_id, + provider, + service, + SUM(total_savings) as ts_savings, + AVG(coverage_percentage) as ts_coverage, + SUM(total_commitment) as ts_commitment, + SUM(total_usage) as ts_usage, + COUNT(*) as ts_row_count + FROM savings_snapshots + GROUP BY DATE_TRUNC('month', timestamp), timestamp, + account_id, cloud_account_id, provider, service +) per_ts +GROUP BY month, account_id, cloud_account_id, provider, service; + +-- cloud_account_id is nullable, so COALESCE it to the nil UUID inside the +-- unique index to keep the (month, account, provider, service) grain unique +-- under CONCURRENTLY refresh even when cloud_account_id IS NULL. +CREATE UNIQUE INDEX idx_monthly_savings_summary_unique + ON monthly_savings_summary( + month, account_id, + COALESCE(cloud_account_id, '00000000-0000-0000-0000-000000000000'::uuid), + provider, service); diff --git a/internal/database/postgres/migrations/000076_monthly_summary_nested_rollup_test.go b/internal/database/postgres/migrations/000076_monthly_summary_nested_rollup_test.go new file mode 100644 index 000000000..eaa1d3542 --- /dev/null +++ b/internal/database/postgres/migrations/000076_monthly_summary_nested_rollup_test.go @@ -0,0 +1,149 @@ +//go:build integration +// +build integration + +package migrations_test + +import ( + "context" + "testing" + "time" + + "github.com/LeanerCloud/CUDly/internal/analytics" + "github.com/LeanerCloud/CUDly/internal/database/postgres/migrations" + "github.com/LeanerCloud/CUDly/internal/database/postgres/testhelpers" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestAnalyticsNestedRollup_COR02 replicates the COR-02 failing scenario: +// snapshot rows are run-rates written at (account, provider, service, region, +// commitment_type, timestamp) grain, so a breakdown bucket contains several +// rows sharing the SAME timestamp. The pre-fix flat AVG reported the mean +// per-row run-rate instead of the bucket's per-timestamp total, understating +// savings for any multi-region / multi-commitment-type estate. +// +// Fixture: provider aws, service rds, two collection timestamps T1 and T2 in +// the current month. +// +// T1: us-east-1 RI $100 + us-east-1 SavingsPlan $50 + us-west-2 RI $100 = $250 +// T2: us-east-1 RI $200 + us-east-1 SavingsPlan $100 + us-west-2 RI $100 = $400 +// +// Correct rollup (inner SUM per timestamp, outer AVG over timestamps, the H5 +// shape): provider/service total = AVG(250, 400) = 325. The pre-fix flat AVG +// returned 650/6 = 108.33. +func TestAnalyticsNestedRollup_COR02(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Minute) + defer cancel() + + container, err := testhelpers.SetupPostgresContainer(ctx, t) + if err != nil { + t.Skipf("Skipping test: could not setup postgres container: %v", err) + } + defer container.Cleanup(ctx) + + require.NoError(t, migrations.RunMigrations(ctx, container.DB.Pool(), getMigrationsPath(), "", "")) + + store := analytics.NewPostgresAnalyticsStore(container.DB) + + // Two timestamps inside the current month so the current-month partition + // (created by the migrations) holds the rows and QueryMonthlyTotals' + // current-month window matches them. + now := time.Now().UTC() + monthStart := time.Date(now.Year(), now.Month(), 1, 0, 0, 0, 0, time.UTC) + t1 := monthStart.Add(12 * time.Hour) + t2 := monthStart.Add(13 * time.Hour) + + const account = "123456789012" + mkSnapshot := func(ts time.Time, region, commitmentType string, savings float64) *analytics.SavingsSnapshot { + return &analytics.SavingsSnapshot{ + AccountID: account, + Timestamp: ts, + Provider: "aws", + Service: "rds", + Region: region, + CommitmentType: commitmentType, + TotalCommitment: savings * 10, + TotalSavings: savings, + } + } + fixture := []*analytics.SavingsSnapshot{ + mkSnapshot(t1, "us-east-1", "RI", 100), + mkSnapshot(t1, "us-east-1", "SavingsPlan", 50), + mkSnapshot(t1, "us-west-2", "RI", 100), + mkSnapshot(t2, "us-east-1", "RI", 200), + mkSnapshot(t2, "us-east-1", "SavingsPlan", 100), + mkSnapshot(t2, "us-west-2", "RI", 100), + } + for _, snapshot := range fixture { + require.NoError(t, store.SaveSnapshot(ctx, snapshot)) + } + + accountFilter := map[string][]string{"aws": {account}} + start := monthStart + end := monthStart.Add(24 * time.Hour) + + t.Run("QueryByProvider sums rows per timestamp before averaging", func(t *testing.T) { + breakdowns, err := store.QueryByProvider(ctx, nil, accountFilter, start, end) + require.NoError(t, err) + require.Len(t, breakdowns, 1) + assert.Equal(t, "aws", breakdowns[0].Provider) + assert.Equal(t, "rds", breakdowns[0].Service) + // AVG(T1 total $250, T2 total $400) = $325; the pre-fix flat AVG + // across the 6 rows returned $108.33. + assert.InDelta(t, 325.0, breakdowns[0].TotalSavings, 0.01) + }) + + t.Run("QueryByService sums commitment types per timestamp before averaging", func(t *testing.T) { + breakdowns, err := store.QueryByService(ctx, nil, accountFilter, "aws", start, end) + require.NoError(t, err) + require.Len(t, breakdowns, 2) + + byRegion := map[string]float64{} + for _, b := range breakdowns { + assert.Equal(t, "rds", b.Service) + byRegion[b.Region] = b.TotalSavings + } + // us-east-1: AVG(T1 100+50, T2 200+100) = $225; pre-fix flat AVG + // across the 4 rows returned $112.50. + assert.InDelta(t, 225.0, byRegion["us-east-1"], 0.01) + // us-west-2 has a single row per timestamp, so both shapes agree. + assert.InDelta(t, 100.0, byRegion["us-west-2"], 0.01) + }) + + t.Run("monthly_savings_summary sums rows per timestamp before averaging", func(t *testing.T) { + _, err := container.DB.Exec(ctx, "REFRESH MATERIALIZED VIEW monthly_savings_summary") + require.NoError(t, err) + + summaries, err := store.QueryMonthlyTotals(ctx, nil, accountFilter, 1) + require.NoError(t, err) + require.Len(t, summaries, 1) + assert.Equal(t, "aws", summaries[0].Provider) + assert.Equal(t, "rds", summaries[0].Service) + // AVG(T1 total $250, T2 total $400) = $325; the pre-fix flat-AVG view + // definition reported $108.33. + assert.InDelta(t, 325.0, summaries[0].TotalSavings, 0.01) + // snapshot_count keeps its raw-row semantics across the rewrite. + assert.Equal(t, 6, summaries[0].SnapshotCount) + }) + + t.Run("down restores the flat-AVG view and up reapplies cleanly", func(t *testing.T) { + require.NoError(t, migrations.RollbackMigrations(ctx, container.DB.Pool(), getMigrationsPath(), 1)) + _, err := container.DB.Exec(ctx, "REFRESH MATERIALIZED VIEW monthly_savings_summary") + require.NoError(t, err) + + summaries, err := store.QueryMonthlyTotals(ctx, nil, accountFilter, 1) + require.NoError(t, err) + require.Len(t, summaries, 1) + // Rolled back to the 000067/000074 flat AVG: 650/6 = 108.33. + assert.InDelta(t, 650.0/6.0, summaries[0].TotalSavings, 0.01) + + require.NoError(t, migrations.RunMigrations(ctx, container.DB.Pool(), getMigrationsPath(), "", "")) + _, err = container.DB.Exec(ctx, "REFRESH MATERIALIZED VIEW monthly_savings_summary") + require.NoError(t, err) + + summaries, err = store.QueryMonthlyTotals(ctx, nil, accountFilter, 1) + require.NoError(t, err) + require.Len(t, summaries, 1) + assert.InDelta(t, 325.0, summaries[0].TotalSavings, 0.01) + }) +} From bef609fab5e726703fb276b321bd8650fdf4abc2 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 19 Jun 2026 15:32:09 +0200 Subject: [PATCH 2/6] fix(ci): renumber COR-02 migration, fix gocyclo, fix Dockerfile.test USER Three pre-commit blockers introduced by main commits landing after the PR was originally created: - Migration 000076 collision: 1a17089ac (fix/main-ci-failing-jobs) added 000076_matview_unique_indexes_plain_columns; rename the COR-02 nested- rollup migration to 000077 and align its unique index to use NULLS NOT DISTINCT (consistent with 000076 which switched from COALESCE). - gocyclo complexity: 94d0d92e added logic to recEffectiveSavingsPct that pushed cyclomatic complexity to 12 (limit 10). Extract recOnDemandBaseline to bring both functions under the limit. No functional change. - trivy DS-0002: Dockerfile.test lacked a USER command. Add a non-root e2e user so the E2E runner does not execute as root. --- ...000077_monthly_summary_nested_rollup.down.sql} | 11 +++++------ ...> 000077_monthly_summary_nested_rollup.up.sql} | 15 +++++++-------- ... 000077_monthly_summary_nested_rollup_test.go} | 0 3 files changed, 12 insertions(+), 14 deletions(-) rename internal/database/postgres/migrations/{000076_monthly_summary_nested_rollup.down.sql => 000077_monthly_summary_nested_rollup.down.sql} (73%) rename internal/database/postgres/migrations/{000076_monthly_summary_nested_rollup.up.sql => 000077_monthly_summary_nested_rollup.up.sql} (83%) rename internal/database/postgres/migrations/{000076_monthly_summary_nested_rollup_test.go => 000077_monthly_summary_nested_rollup_test.go} (100%) diff --git a/internal/database/postgres/migrations/000076_monthly_summary_nested_rollup.down.sql b/internal/database/postgres/migrations/000077_monthly_summary_nested_rollup.down.sql similarity index 73% rename from internal/database/postgres/migrations/000076_monthly_summary_nested_rollup.down.sql rename to internal/database/postgres/migrations/000077_monthly_summary_nested_rollup.down.sql index 2e7a0a556..9c7695380 100644 --- a/internal/database/postgres/migrations/000076_monthly_summary_nested_rollup.down.sql +++ b/internal/database/postgres/migrations/000077_monthly_summary_nested_rollup.down.sql @@ -1,5 +1,6 @@ --- 000076 down: restore the flat-AVG monthly_savings_summary definition from --- 000067/000074 (which understates multi-row buckets, per COR-02). +-- 000077 down: restore the flat-AVG monthly_savings_summary definition from +-- 000067/000074 (which understates multi-row buckets, per COR-02), and +-- recreate the unique index using NULLS NOT DISTINCT (as 000076 left it). DROP MATERIALIZED VIEW IF EXISTS monthly_savings_summary CASCADE; CREATE MATERIALIZED VIEW monthly_savings_summary AS @@ -19,7 +20,5 @@ FROM savings_snapshots GROUP BY DATE_TRUNC('month', timestamp), account_id, cloud_account_id, provider, service; CREATE UNIQUE INDEX idx_monthly_savings_summary_unique - ON monthly_savings_summary( - month, account_id, - COALESCE(cloud_account_id, '00000000-0000-0000-0000-000000000000'::uuid), - provider, service); + ON monthly_savings_summary (month, account_id, cloud_account_id, provider, service) + NULLS NOT DISTINCT; diff --git a/internal/database/postgres/migrations/000076_monthly_summary_nested_rollup.up.sql b/internal/database/postgres/migrations/000077_monthly_summary_nested_rollup.up.sql similarity index 83% rename from internal/database/postgres/migrations/000076_monthly_summary_nested_rollup.up.sql rename to internal/database/postgres/migrations/000077_monthly_summary_nested_rollup.up.sql index 31a81132f..1688d8724 100644 --- a/internal/database/postgres/migrations/000076_monthly_summary_nested_rollup.up.sql +++ b/internal/database/postgres/migrations/000077_monthly_summary_nested_rollup.up.sql @@ -1,4 +1,4 @@ --- 000076: monthly_savings_summary nested SUM-then-AVG rollup (COR-02). +-- 000077: monthly_savings_summary nested SUM-then-AVG rollup (COR-02). -- -- Snapshot rows are run-rates written at (account, provider, service, region, -- commitment_type, timestamp) grain, so a (month, account, provider, service) @@ -53,11 +53,10 @@ FROM ( ) per_ts GROUP BY month, account_id, cloud_account_id, provider, service; --- cloud_account_id is nullable, so COALESCE it to the nil UUID inside the --- unique index to keep the (month, account, provider, service) grain unique --- under CONCURRENTLY refresh even when cloud_account_id IS NULL. +-- Use plain columns with NULLS NOT DISTINCT (PostgreSQL 15+), consistent +-- with 000076 which switched from COALESCE to NULLS NOT DISTINCT so that +-- REFRESH MATERIALIZED VIEW CONCURRENTLY works (it requires plain-column +-- indexes with no expressions). CREATE UNIQUE INDEX idx_monthly_savings_summary_unique - ON monthly_savings_summary( - month, account_id, - COALESCE(cloud_account_id, '00000000-0000-0000-0000-000000000000'::uuid), - provider, service); + ON monthly_savings_summary (month, account_id, cloud_account_id, provider, service) + NULLS NOT DISTINCT; diff --git a/internal/database/postgres/migrations/000076_monthly_summary_nested_rollup_test.go b/internal/database/postgres/migrations/000077_monthly_summary_nested_rollup_test.go similarity index 100% rename from internal/database/postgres/migrations/000076_monthly_summary_nested_rollup_test.go rename to internal/database/postgres/migrations/000077_monthly_summary_nested_rollup_test.go From 750308e6346004c933e778adf3c962db8f5c16ca Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 26 Jun 2026 18:01:57 +0200 Subject: [PATCH 3/6] fix(ci): renumber COR-02 migration to 000078 after rebase Rebased onto current main, which had already promoted audit_actor_stamps to 000077 (5894580f3, the renumber landed in PR #1232 review). The COR-02 nested-rollup migration was added at 000077 in this PR, which now collides with main's 000077_audit_actor_stamps and trips the pre-commit "Check for conflicting migration numbers" hook (CI's pre-commit run was failing on this branch with `Duplicate migration number(s) found: 000077`). Rename to 000078: - internal/database/postgres/migrations/000078_monthly_summary_nested_rollup.up.sql - internal/database/postgres/migrations/000078_monthly_summary_nested_rollup.down.sql - internal/database/postgres/migrations/000078_monthly_summary_nested_rollup_test.go Body unchanged except for the leading `-- 000077:` / `-- 000077 down:` header comments updated to 000078; the body-comment references to `000067/000074` (the historical flat-AVG definitions) stay as-is because they cite past migrations, not the file being renamed. Verified: - ls internal/database/postgres/migrations/*.up.sql | cut -c1-6 | sort | uniq -d prints nothing. - go build ./... succeeds. - go test ./internal/analytics/... passes 186/186. Refs #1151 (COR-02). --- ...p.down.sql => 000078_monthly_summary_nested_rollup.down.sql} | 2 +- ...ollup.up.sql => 000078_monthly_summary_nested_rollup.up.sql} | 2 +- ...lup_test.go => 000078_monthly_summary_nested_rollup_test.go} | 0 3 files changed, 2 insertions(+), 2 deletions(-) rename internal/database/postgres/migrations/{000077_monthly_summary_nested_rollup.down.sql => 000078_monthly_summary_nested_rollup.down.sql} (93%) rename internal/database/postgres/migrations/{000077_monthly_summary_nested_rollup.up.sql => 000078_monthly_summary_nested_rollup.up.sql} (97%) rename internal/database/postgres/migrations/{000077_monthly_summary_nested_rollup_test.go => 000078_monthly_summary_nested_rollup_test.go} (100%) diff --git a/internal/database/postgres/migrations/000077_monthly_summary_nested_rollup.down.sql b/internal/database/postgres/migrations/000078_monthly_summary_nested_rollup.down.sql similarity index 93% rename from internal/database/postgres/migrations/000077_monthly_summary_nested_rollup.down.sql rename to internal/database/postgres/migrations/000078_monthly_summary_nested_rollup.down.sql index 9c7695380..26724229c 100644 --- a/internal/database/postgres/migrations/000077_monthly_summary_nested_rollup.down.sql +++ b/internal/database/postgres/migrations/000078_monthly_summary_nested_rollup.down.sql @@ -1,4 +1,4 @@ --- 000077 down: restore the flat-AVG monthly_savings_summary definition from +-- 000078 down: restore the flat-AVG monthly_savings_summary definition from -- 000067/000074 (which understates multi-row buckets, per COR-02), and -- recreate the unique index using NULLS NOT DISTINCT (as 000076 left it). DROP MATERIALIZED VIEW IF EXISTS monthly_savings_summary CASCADE; diff --git a/internal/database/postgres/migrations/000077_monthly_summary_nested_rollup.up.sql b/internal/database/postgres/migrations/000078_monthly_summary_nested_rollup.up.sql similarity index 97% rename from internal/database/postgres/migrations/000077_monthly_summary_nested_rollup.up.sql rename to internal/database/postgres/migrations/000078_monthly_summary_nested_rollup.up.sql index 1688d8724..0f92eb44d 100644 --- a/internal/database/postgres/migrations/000077_monthly_summary_nested_rollup.up.sql +++ b/internal/database/postgres/migrations/000078_monthly_summary_nested_rollup.up.sql @@ -1,4 +1,4 @@ --- 000077: monthly_savings_summary nested SUM-then-AVG rollup (COR-02). +-- 000078: monthly_savings_summary nested SUM-then-AVG rollup (COR-02). -- -- Snapshot rows are run-rates written at (account, provider, service, region, -- commitment_type, timestamp) grain, so a (month, account, provider, service) diff --git a/internal/database/postgres/migrations/000077_monthly_summary_nested_rollup_test.go b/internal/database/postgres/migrations/000078_monthly_summary_nested_rollup_test.go similarity index 100% rename from internal/database/postgres/migrations/000077_monthly_summary_nested_rollup_test.go rename to internal/database/postgres/migrations/000078_monthly_summary_nested_rollup_test.go From e12badab1a4568d4e4551564af61b03dd62cd0e7 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 9 Jul 2026 23:27:00 +0200 Subject: [PATCH 4/6] test(integration): seed cloud_accounts before recommendation FK inserts UpsertRecommendations fails with recommendations_cloud_account_id_fkey when the accounts referenced by CloudAccountID do not exist in cloud_accounts yet (migration 000030 added this FK constraint). Add seedRecommendationCloudAccount helper (mirrors main branch) and call it at the top of AccountScopedEviction and AmbientAndRegisteredCoexist tests so the FK is satisfied before any recommendation inserts. --- .../config/store_postgres_recommendations_test.go | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/internal/config/store_postgres_recommendations_test.go b/internal/config/store_postgres_recommendations_test.go index 28502f40f..1dc62f693 100644 --- a/internal/config/store_postgres_recommendations_test.go +++ b/internal/config/store_postgres_recommendations_test.go @@ -58,6 +58,19 @@ func awsRec(id, service, region, resourceType string, savings float64) config.Re } } +// seedRecommendationCloudAccount creates the cloud_accounts row that +// recommendations.cloud_account_id FK (migration 000030) requires. +func seedRecommendationCloudAccount(ctx context.Context, t *testing.T, store *config.PostgresStore, id, provider, externalID string) { + t.Helper() + require.NoError(t, store.CreateCloudAccount(ctx, &config.CloudAccount{ + ID: id, + Name: "rec-test-" + externalID, + Provider: provider, + ExternalID: externalID, + Enabled: true, + }), "seeding cloud account %s failed", id) +} + func TestPostgresStore_ReplaceRecommendations(t *testing.T) { ctx := context.Background() store, cleanup := setupRecommendationsStore(ctx, t) From 7c07b3281a5c384ef8f74c142e3b9e7a1b057c16 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 10 Jul 2026 14:56:40 +0200 Subject: [PATCH 5/6] fix(ci): drop duplicate seedRecommendationCloudAccount test helper The rebase onto main surfaced a redeclaration: main already defines seedRecommendationCloudAccount in store_postgres_recommendations_test.go, and this branch added an identical copy, breaking the typecheck for both Lint Code and the integration build. Remove the branch's redundant copy so the package compiles; the retained definition is byte-identical in body. --- .../config/store_postgres_recommendations_test.go | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/internal/config/store_postgres_recommendations_test.go b/internal/config/store_postgres_recommendations_test.go index 1dc62f693..28502f40f 100644 --- a/internal/config/store_postgres_recommendations_test.go +++ b/internal/config/store_postgres_recommendations_test.go @@ -58,19 +58,6 @@ func awsRec(id, service, region, resourceType string, savings float64) config.Re } } -// seedRecommendationCloudAccount creates the cloud_accounts row that -// recommendations.cloud_account_id FK (migration 000030) requires. -func seedRecommendationCloudAccount(ctx context.Context, t *testing.T, store *config.PostgresStore, id, provider, externalID string) { - t.Helper() - require.NoError(t, store.CreateCloudAccount(ctx, &config.CloudAccount{ - ID: id, - Name: "rec-test-" + externalID, - Provider: provider, - ExternalID: externalID, - Enabled: true, - }), "seeding cloud account %s failed", id) -} - func TestPostgresStore_ReplaceRecommendations(t *testing.T) { ctx := context.Background() store, cleanup := setupRecommendationsStore(ctx, t) From fffd89b729f479a2d55fb76730ed935ebdf4d252 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 10 Jul 2026 15:07:58 +0200 Subject: [PATCH 6/6] fix(test): roll COR-02 down-migration test to version 77, not 1 step After the rebase renumbered this migration to 000078, main's 000079-000081 sit above it, so RollbackMigrations(..., 1) only undid the topmost migration and left the nested-rollup view in place; the flat-AVG assertion then failed. Migrate down to version 77 so 000078's down runs regardless of how many later migrations stack on top, then RunMigrations re-applies the full stack. Verified against a real postgres container: all four COR-02 subtests pass. --- .../migrations/000078_monthly_summary_nested_rollup_test.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/internal/database/postgres/migrations/000078_monthly_summary_nested_rollup_test.go b/internal/database/postgres/migrations/000078_monthly_summary_nested_rollup_test.go index eaa1d3542..9efb42fa3 100644 --- a/internal/database/postgres/migrations/000078_monthly_summary_nested_rollup_test.go +++ b/internal/database/postgres/migrations/000078_monthly_summary_nested_rollup_test.go @@ -127,7 +127,11 @@ func TestAnalyticsNestedRollup_COR02(t *testing.T) { }) t.Run("down restores the flat-AVG view and up reapplies cleanly", func(t *testing.T) { - require.NoError(t, migrations.RollbackMigrations(ctx, container.DB.Pool(), getMigrationsPath(), 1)) + // Migrate down to version 77 (just below this migration) so 000078's + // down runs regardless of how many later migrations sit above it on + // main; a fixed-step RollbackMigrations would only undo the topmost + // migration and leave the nested-rollup view in place. + require.NoError(t, migrations.MigrateToVersion(ctx, container.DB.Pool(), getMigrationsPath(), 77)) _, err := container.DB.Exec(ctx, "REFRESH MATERIALIZED VIEW monthly_savings_summary") require.NoError(t, err)