Skip to content

perf(config): index the active purchase-history expiry filter - #942

Merged
cristim merged 2 commits into
LeanerCloud:mainfrom
IAmJSD:fix/issue-33
Oct 16, 2026
Merged

cristim merged 2 commits into
LeanerCloud:mainfrom
IAmJSD:fix/issue-33

Conversation

@IAmJSD

@IAmJSD IAmJSD commented Oct 16, 2026

Copy link
Copy Markdown
Collaborator

Root cause

The issue still applies to current main. Since it was filed, the predicate gained an inclusive >= and revoked_at IS NULL, but the expiry part is the same:

WHERE term > 0 AND timestamp + make_interval(hours => term * 8760) >= $1 AND revoked_at IS NULL

The expression uses two columns. timestamptz + interval is 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

  • GetActivePurchaseHistory now 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), extract on 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.
  • Migration 000126 idx_purchase_history_active_expiry indexes 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).
    • The index is not partial. Postgres ignores expression statistics on a partial index, and without them it estimated a third of the table as active and kept the full scan.
    • numeric can't overflow, so no term value can make an INSERT (or the CREATE INDEX on existing data) fail on the index expression. An int4 or interval form could.
  • I didn't use the issue's other suggestion (AND timestamp > $1 - interval '3 years'). term has no CHECK constraint, so a hard 3-year bound could silently drop rows with longer terms and change the result.
  • appendAccountPredicate is 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 with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null because this machine's default AWS profile is invalid.

Same result set (Postgres integration test)

TestGetActivePurchaseHistory_MatchesLegacyPredicate seeds boundary rows:

  • expiry exactly at asOf, and ±1µs
  • 1y and 3y terms
  • calendar years that include 2024-02-29, where a calendar year is longer than 8760h
  • timestamps written in America/New_York
  • term 0, -1 and 50
  • a revoked row and a future-dated row

It then compares GetActivePurchaseHistory with 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_SessionTimeZoneIndependent runs the new query with session TimeZone = 'America/New_York' on a US DST-change day and checks it still matches.

Index use (regression test, fail-before / pass-after)

TestGetActivePurchaseHistory_UsesExpiryIndex seeds 50k rows over 20 years (about 10% active), runs ANALYZE, then runs EXPLAIN (FORMAT JSON) on the exact query, both unscoped and account-scoped. It asserts an Index Cond on idx_purchase_history_active_expiry.

go test -tags integration -count=1 -v -run 'TestGetActivePurchaseHistory_' ./internal/config/
go test -tags integration -count=1 -v -run TestMigration_PurchaseHistoryActiveExpiryIndex ./internal/database/postgres/migrations/
state MatchesLegacy SessionTZ UsesExpiryIndex
pre-fix (old predicate, no 000126) PASS (trivially) PASS FAIL (plan: Index Scan using idx_purchase_history_timestamp + Filter)
new predicate, 000126 removed FAIL
old predicate, 000126 present PASS PASS FAIL (index unusable)
this PR PASS PASS PASS
mutation: 364-day year (31449600) FAIL FAIL FAIL
mutation: > instead of >= FAIL FAIL PASS
mutation: AT TIME ZONE 'America/New_York' on the row side FAIL FAIL FAIL

TestMigration_PurchaseHistoryActiveExpiryIndex steps 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 with term = 2147483647 still 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):

Index Scan using idx_purchase_history_timestamp on purchase_history  (cost=0.42..66438.43 rows=329878 width=876) (actual time=0.027..270.516 rows=99431 loops=1)
  Filter: ((revoked_at IS NULL) AND (term > 0) AND (("timestamp" + make_interval(0, 0, 0, 0, (term * 8760), 0, '0'::double precision)) >= '2026-10-16 00:00:00+00'::timestamp with time zone))
  Rows Removed by Filter: 900569
Execution Time: 274.617 ms   (first run 434.7 ms)

After (this PR):

Gather Merge  (cost=27579.42..37466.44 rows=84740 width=876) (actual time=35.515..57.546 rows=99431 loops=1)
  ->  Sort  (Sort Key: "timestamp" DESC)
        ->  Parallel Index Scan using idx_purchase_history_active_expiry on purchase_history  (actual time=0.031..15.614 rows=33144 loops=3)
              Index Cond: ((EXTRACT(epoch FROM ("timestamp" AT TIME ZONE 'UTC'::text)) + ((term)::numeric * '31536000'::numeric)) >= 1792108800.000000)
              Filter: ((revoked_at IS NULL) AND (term > 0))
              Rows Removed by Filter: 167
Execution Time: 61.986 ms

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_timestamp scan, so the change doesn't slow anything down.

Other checks

  • go build ./...; go vet (with and without -tags integration) on the touched packages
  • go test ./internal/config/... ./internal/api/... ./internal/analytics/... ./internal/database/...: all pass
  • go test -tags integration ./internal/api/ -run 'NullablePurchaseCost|Integration': pass (an existing GetActivePurchaseHistory caller)
  • golangci-lint v2.10.1 (--allow-parallel-runners, plain and --build-tags integration): 0 issues
  • gocyclo -over 10 on every non-test file: silent
  • gofmt clean; gosec hook (scripts/gosec-hook.sh) passes; no duplicate migration numbers. git-secrets isn't installed on this machine, so that hook didn't run.
  • Full go test -tags integration ./internal/database/postgres/migrations/: everything passed except TestEnsureAdminUser_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 with fork/fix/issue-24 (9e46f8f):

Out of scope / notes

  • The migration uses a plain CREATE INDEX inside 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.
  • Unrelated: scanPurchaseHistoryRow scans ramp_step into *int, but the column is nullable. A row inserted outside SavePurchaseHistory with NULL ramp_step makes every purchase-history read fail at Scan. I didn't change this; the test fixtures set ramp_step = 0.
  • The interfaces.go doc for GetActivePurchaseHistory describes 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

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>
@coderabbitai

coderabbitai Bot commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Note

Currently processing new changes in this PR. This may take a few minutes, please wait...

⚙️ Run configuration
  • Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 36283b2e-6cbb-40a2-af2a-c703005d9f1b
📥 Commits

Reviewing files that changed from the base of the PR and between 3f83cba and 1960cd9.

📒 Files selected for processing: 6
  • internal/config/store_postgres.go
  • internal/config/store_postgres_active_history_integration_test.go
  • internal/config/store_postgres_pgxmock_test.go
  • internal/database/postgres/migrations/000126_purchase_history_active_expiry_idx.down.sql
  • internal/database/postgres/migrations/000126_purchase_history_active_expiry_idx.up.sql
  • internal/database/postgres/migrations/000126_purchase_history_active_expiry_idx_test.go
+--------------------------------------------------------------------------------------------------------------------------------------------------------------+
| Find bugs once. Once a human tester finds a bug, it should be the last time a human tester finds that bug. Automatic tests should check for it from then on. |
+--------------------------------------------------------------------------------------------------------------------------------------------------------------+
  \
   \   (\__/)
       (•ㅅ•)
       /   づ
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

@cristim cristim added impact/internal Team-internal only priority/p3 Polish / idea / may never ship severity/low Minor harm urgency/eventually No deadline effort/xs Trivial / one-liner type/bug Defect triaged Item has been triaged labels Oct 16, 2026
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>
@IAmJSD

IAmJSD commented Oct 16, 2026

Copy link
Copy Markdown
Collaborator Author

#931 has merged, so I merged origin/main into this branch at 1960cd9. This resolves the trial-merge conflict noted in the description. I merged instead of rebasing, so there was no force-push.

Conflict resolution: internal/config/store_postgres_pgxmock_test.go, on the expected GetActivePurchaseHistory query regex only. It now combines this PR's epoch-based expiry predicate (activeExpiryEpochExpr) with #931's (provider = '' OR LOWER(provider) = LOWER($3)) match. store_postgres.go auto-merged and contains both changes. Migration 000126 is still the next free number (main ends at 000125).

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 cristim left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. Mark ready for review (author, external contributor).
  2. Re-confirm the migration number is still free at merge time (next after 000125 is 000126 today).
  3. Resolve the #931 test-file conflict by rebase if #931 lands first; the verdict restarts on any new commit.
  4. Maintainer review of the verdict on the final SHA, then merge normally.

@cristim

cristim commented Oct 16, 2026

Copy link
Copy Markdown
Member

Gate record for 1960cd9 (origin/main CLAUDE.md "Review gate"):

  • Independent review of the PR's own diff at 1960cd9 (a merge of origin/main on top of 1e299bb): no blocking findings.
    • Migration 000126 is the next free number, and no other open PR adds a migration.
    • The index expression matches the expiry predicate in activePurchaseHistoryQuery (internal/config/store_postgres.go).
    • Non-blocking: the plain CREATE INDEX briefly blocks writes to the append-only table. CONCURRENTLY is not possible because the runner wraps migrations in a transaction (migrate.go, around line 353).
  • CI on 1960cd9: CI Success and Run pre-commit hooks pass.
  • Local verification (macOS, testcontainers Postgres 16):
    • TestGetActivePurchaseHistory_UsesExpiryIndex EXPLAINs the exact query on 50k rows, scoped and unscoped, and requires an Index Cond on idx_purchase_history_active_expiry.
    • The legacy-predicate, session-timezone and migration (125 -> 126 -> 125 -> 126) tests all pass.
    • The internal/config and internal/api suites pass.
  • CR skipped: draft and quota-limited; covered by the independent review plus local verification.
  • Labels mirrored from PERF-03: GetActivePurchaseHistory predicate non-sargable; full table scan every analytics cycle #33.

Marking ready (owner-authorized) and merging at 1960cd9 with --match-head-commit. #931 touches the same pgxmock test file and will need a rebase.

@cristim
cristim marked this pull request as ready for review October 16, 2026 10:51
@cristim
cristim merged commit b49b494 into LeanerCloud:main Oct 16, 2026
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/xs Trivial / one-liner impact/internal Team-internal only priority/p3 Polish / idea / may never ship severity/low Minor harm triaged Item has been triaged type/bug Defect urgency/eventually No deadline

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PERF-03: GetActivePurchaseHistory predicate non-sargable; full table scan every analytics cycle

2 participants