Repository navigation
perf(config): index the active purchase-history expiry filter - #942
Conversation
GetActivePurchaseHistory filtered on timestamp + make_interval(hours => term * 8760) >= $1. That spans two columns and timestamptz + interval is only STABLE, so no index could serve it and every analytics cycle, dashboard load and inventory call read and filtered all history ever recorded. Compare the same expiry as Unix epoch seconds instead, built only from IMMUTABLE parts (UTC wall time, epoch, numeric arithmetic), and index that expression in migration 000126. For an hours-only interval the arithmetic is exact, so the active set is unchanged; numeric cannot overflow, so no term value can make an INSERT fail on the index. An integration test compares the new query against the old predicate on boundary data (exact expiry, +-1us, leap-day calendar years, DST zones, revoked, non-positive and long terms, scoped and unscoped) and asserts the planner uses the new index on a mostly-expired table. Closes LeanerCloud#33 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configuration
📒 Files selected for processing: 6
✨ Finishing Touches
Comment |
Resolve pgxmock test conflict with LeanerCloud#931: the expected GetActivePurchaseHistory query keeps this PR's epoch-based expiry predicate and LeanerCloud#931's case-insensitive/empty provider match. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
#931 has merged, so I merged Conflict resolution: Checks on macOS at 1960cd9, with AWS config and credentials set to /dev/null:
The fork CI run for the new head needs approval. 🤖 Generated with Claude Code |
cristim
left a comment
There was a problem hiding this comment.
Independent review of 1960cd9: draft: blocks ready
Reviewed the PR's own change only (merge-base 3f83cba...1960cd9: 6 files, 363 additions; the merge-in of main was not reviewed).
Findings
- No BLOCKING findings.
- NON-BLOCKING: plain CREATE INDEX (000126 up.sql:18) takes a write-blocking lock on purchase_history while it builds. This is correct: internal/database/postgres/migrations/migrate.go runs every migration in a transaction (see the comment near migrate.go:353), where CREATE INDEX CONCURRENTLY would fail. The table is append-only, so the lock should be brief. Already noted in the PR body.
- NON-BLOCKING: the interface doc and activeExpiryEpochExpr must stay in sync with the migration by hand. The migration test pins the index definition and the integration tests fail if they diverge, so the risk is covered.
What I attacked
- Migration number: origin/main ends at 000125 (lock_default_user_groups); no 000126+ on main, and none of the open PRs adds a migration other than #942. No gap or ordering problem.
- Query vs index: the store builds its SQL in activePurchaseHistoryQuery (internal/config/store_postgres.go, called by GetActivePurchaseHistory). Its expiry predicate uses the same expression text as the index. TestGetActivePurchaseHistory_UsesExpiryIndex EXPLAINs that exact query on 50k rows (unscoped and account-scoped) and requires an Index Cond on idx_purchase_history_active_expiry. I reran it on Postgres 16 via testcontainers: pass.
- Result-set equivalence vs the legacy predicate (boundary, DST, leap-year, timezone, revoked, term<=0 rows): TestGetActivePurchaseHistory_MatchesLegacyPredicate and SessionTimeZoneIndependent pass.
- Down migration: DROP INDEX IF EXISTS; TestMigration_PurchaseHistoryActiveExpiryIndex steps 125 -> 126 -> 125 -> 126 and passes. It also checks the index is not partial and that term=2147483647 still inserts.
- go test ./internal/config/ ./internal/api/ at this SHA: pass (3953 tests).
Gate status
- CI on 1960cd9: all checks pass (Unit, Integration, E2E, Lint, pre-commit, Security, Build, Trivy, gosec). Snyk skipped. CodeRabbit skipped (draft).
- Labels match #33: impact/internal, triaged, priority/p3, severity/low, urgency/eventually, effort/xs, type/bug.
- Local verification: real Postgres 16 container, realistic fixtures, fail-before/pass-after per the PR body (not re-run by me for the pre-fix state). No real-account verification applies.
- Overlap with #931: the PR body reports a textual conflict in store_postgres_pgxmock_test.go; whoever merges second must resolve it.
Blocks ready
- Mark ready for review (author, external contributor).
- Re-confirm the migration number is still free at merge time (next after 000125 is 000126 today).
- Resolve the #931 test-file conflict by rebase if #931 lands first; the verdict restarts on any new commit.
- Maintainer review of the verdict on the final SHA, then merge normally.
|
Gate record for 1960cd9 (origin/main CLAUDE.md "Review gate"):
Marking ready (owner-authorized) and merging at 1960cd9 with --match-head-commit. #931 touches the same pgxmock test file and will need a rebase. |
Root cause
The issue still applies to current main. Since it was filed, the predicate gained an inclusive
>=andrevoked_at IS NULL, but the expiry part is the same:The expression uses two columns.
timestamptz + intervalis only STABLE (day and month intervals depend on the session time zone), so neither a plain index nor an expression index can serve it. Every analytics collection, dashboard KPI load and/api/inventory/*call reads and filters every purchase_history row ever recorded.Fix
GetActivePurchaseHistorynow compares the same expiry as Unix epoch seconds:(extract(epoch FROM (timestamp AT TIME ZONE 'UTC')) + term::numeric * 31536000) >= extract(epoch FROM ($1::timestamptz AT TIME ZONE 'UTC')).Every part is IMMUTABLE:
timezone(text, timestamptz),extracton a plain timestamp, and numeric arithmetic. For an hours-only interval this is exact arithmetic on the same instant, so the matched rows don't change. 31536000 = 8760 × 3600, the same 365-day year as before. The query is built in a new helper,activePurchaseHistoryQuery, so the tests can EXPLAIN the exact SQL.idx_purchase_history_active_expiryindexes that expression. The down migration drops it. Main ends at 000125, and no open PR adds a migration (checked all 11 open PRs; fix(purchase): pin each execution lineage's fan-out shape #896's 000123 and fix(storage): retain unfinished ramp execution history #904's 000124 are merged).termvalue can make an INSERT (or the CREATE INDEX on existing data) fail on the index expression. An int4 or interval form could.AND timestamp > $1 - interval '3 years').termhas no CHECK constraint, so a hard 3-year bound could silently drop rows with longer terms and change the result.appendAccountPredicateis untouched; only the date/status part of the predicate changed.Verification
All Postgres evidence below is from a local Postgres 16 (testcontainers
postgres:16-alpine, the same major version as CI), not a production database. Go tests were run withAWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/nullbecause this machine's default AWS profile is invalid.Same result set (Postgres integration test)
TestGetActivePurchaseHistory_MatchesLegacyPredicateseeds boundary rows:term0, -1 and 50It then compares
GetActivePurchaseHistorywith the old predicate run as SQL on the same data. The comparison covers every row's own expiry and ±1µs, asOf values in Asia/Tokyo, and three account scopes (unscoped, one provider, and a""provider key plus aws).TestGetActivePurchaseHistory_SessionTimeZoneIndependentruns the new query with sessionTimeZone = 'America/New_York'on a US DST-change day and checks it still matches.Index use (regression test, fail-before / pass-after)
TestGetActivePurchaseHistory_UsesExpiryIndexseeds 50k rows over 20 years (about 10% active), runs ANALYZE, then runsEXPLAIN (FORMAT JSON)on the exact query, both unscoped and account-scoped. It asserts an Index Cond onidx_purchase_history_active_expiry.Index Scan using idx_purchase_history_timestamp+ Filter)31449600)>instead of>=AT TIME ZONE 'America/New_York'on the row sideTestMigration_PurchaseHistoryActiveExpiryIndexsteps through migrations 125 → 126 → 125 → 126 and checks the index is absent, present, dropped and present again. It also checks the index isn't partial and that a row withterm = 2147483647still inserts.EXPLAIN ANALYZE before and after (local Postgres 16.15, seeded table)
The table has 1,000,000 rows spread evenly over the 20 years before 2026-10-16, alternating 1y and 3y terms, 1% revoked; 99,431 are active. The table is 150 MB and the new index is 30 MB. These are warm runs. Production volumes and timings will differ.
Before (old predicate, no new index):
After (this PR):
Rows examined dropped from 1,000,000 to about 99.6k. The planner's row estimate went from 329,878 (a blind one-third guess) to 84,740 (actual 99,431). The gain grows with the share of expired history: with asOf set so about 1.6% of rows are active, the new plan ran in 7.9 ms. When most rows are still active (about 20% here), the planner keeps the ordered
idx_purchase_history_timestampscan, so the change doesn't slow anything down.Other checks
go build ./...;go vet(with and without-tags integration) on the touched packagesgo test ./internal/config/... ./internal/api/... ./internal/analytics/... ./internal/database/...: all passgo test -tags integration ./internal/api/ -run 'NullablePurchaseCost|Integration': pass (an existing GetActivePurchaseHistory caller)--allow-parallel-runners, plain and--build-tags integration): 0 issuesgocyclo -over 10on every non-test file: silentscripts/gosec-hook.sh) passes; no duplicate migration numbers. git-secrets isn't installed on this machine, so that hook didn't run.go test -tags integration ./internal/database/postgres/migrations/: everything passed exceptTestEnsureAdminUser_GroupAssignment, which failed once under parallel container load. It passed when rerun on its own, and it doesn't touch purchase_history.Overlap with #931 (
fix/issue-24)#931 changes the account half of the predicate (
appendAccountPredicate); this PR changes only the expiry half and adds the index. I ran a trial merge of this branch withfork/fix/issue-24(9e46f8f):store_postgres.go,types.goandinterfaces.gomerge cleanly.internal/config/store_postgres_pgxmock_test.go. Both PRs edit the same line: the SQL regexp inTestPGXMock_GetActivePurchaseHistory_AccountScoped, where this PR changes the expiry part and fix(config): match purchase history with an empty or differently cased provider #931 changes the provider part. I couldn't avoid it without keeping the old non-indexable predicate in the SQL. The fix is to keep both edits: this PR's expiry text with fix(config): match purchase history with an empty or differently cased provider #931's((provider = '' OR LOWER(provider) = LOWER(\$3)) AND account_id = ANY(\$4)).go vetgo test ./internal/config/... ./internal/api/... ./internal/analytics/...TestPGXMock_GetPurchaseHistoryFiltered_*andTestPGXMock_GetActivePurchaseHistory_*TestAppendAccountPredicate_ProviderGateandTestAccountPredicate_ProviderCaseAndEmpty_IntegrationLOWER(provider)account predicate.Out of scope / notes
CREATE INDEXinside the migration transaction, like the other index migrations, so writes to purchase_history wait while the index builds. The table is append-only and small, so this should be brief, but large installs should know about it.scanPurchaseHistoryRowscansramp_stepinto*int, but the column is nullable. A row inserted outsideSavePurchaseHistorywith NULLramp_stepmakes every purchase-history read fail at Scan. I didn't change this; the test fixtures setramp_step = 0.interfaces.godoc forGetActivePurchaseHistorydescribes the semantics ("timestamp + term years >= asOf"), which are unchanged, so I left it alone (it also sits next to fix(config): match purchase history with an empty or differently cased provider #931's edits).Maintainer: please mirror the issue's triage labels on this PR:
impact/internal,triaged,priority/p3,severity/low,urgency/eventually,effort/xs,type/bug.Closes #33
🤖 Generated with Claude Code