perf(selfhost): tune Postgres autovacuum and document the observation-write batching decision - #2627
Conversation
…-write batching decision Closes #2543. github_rate_limit_observations receives one INSERT per outbound GitHub API response and is pruned in daily bulk deletes by the retention job -- an insert-then-bulk-delete pattern that is exactly the shape that causes dead-tuple bloat under Postgres's stock autovacuum settings (the codebase already has an alert watching for this symptom generally, but nothing pre-empted it for this specific table). Adds tuneGithubRateLimitObservationsAutovacuum (src/selfhost/pg-adapter.ts), applying a lower autovacuum_vacuum_scale_factor (0.05 vs Postgres's 0.2 default) via the same D1Database.exec() surface runSelfHostMigrations already uses for migrations. Runs once at boot, after migrations (the table must exist first), gated behind the existing usePostgres check -- a no-op on SQLite, which has no autovacuum concept at all. The ALTER is idempotent (re-applying the same storage parameter is a no-op), so it needs no migration-ledger tracking, and best-effort (a failed tune logs and continues rather than blocking boot -- an optimization, never a correctness dependency). Verified against a real Postgres 16 container, not just a mocked interaction test. Evaluated batching the observation write (the issue's second ask) and documented the decision NOT to implement it, in place at the one call site (src/github/backfill.ts): the write rate is bounded by GitHub's own REST budget for a single App installation (~5000/hour, further capped by QUEUE_CONCURRENCY's small worker pool), nowhere near a volume that meaningfully pressures a Postgres connection pool with single-row INSERTs. shouldWaitForGitHubRateLimit reads the LATEST row from this exact table for admission control across every self-host queue worker, including in a multi-instance/shared-Postgres deployment -- a batching window would trade a real but currently-unmeasured write-volume concern for a genuine risk to the admission-control freshness the #1936 rate-limit-reliability campaign this whole roadmap is part of was built to protect.
|
Warning 🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨 ⏸️ Gittensory review result - manual review recommendedReview updated: 2026-07-02 20:33:07 UTC
⏸️ Suggested Action - Manual Review
Review summary Nits — 6 non-blocking
Review context
Contributor next steps
Signal definitions
🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2627 +/- ##
=======================================
Coverage 96.05% 96.05%
=======================================
Files 234 234
Lines 26280 26280
Branches 9531 9531
=======================================
Hits 25244 25244
Misses 425 425
Partials 611 611
🚀 New features to boost your workflow:
|
Summary
Closes #2543.
github_rate_limit_observationsreceives one INSERT per outbound GitHub API response and is pruned in daily bulk deletes by the retention job — an insert-then-bulk-delete pattern that is exactly the shape known to cause dead-tuple bloat under Postgres's stock autovacuum settings (the codebase already has an alert rule watching for this general symptom, which implies the risk was anticipated but not pre-empted for this specific table). Explicitly framed by the issue as a before-larger-scale item, not an active production problem today.What changed
src/selfhost/pg-adapter.ts: newtuneGithubRateLimitObservationsAutovacuum(db), applyingALTER TABLE github_rate_limit_observations SET (autovacuum_vacuum_scale_factor = 0.05, autovacuum_vacuum_threshold = 50)— a lower scale factor than Postgres's 0.2 default, so autovacuum reclaims space promptly after each day's bulk delete instead of letting dead tuples accumulate across cycles. Uses the exact sameD1Database.exec()surfacerunSelfHostMigrationsalready relies on for migrations, so it reuses the existingtranslateDdlSQL path rather than a second raw-pool mechanism.src/server.ts: calls the tuning step once at boot, immediately afterrunSelfHostMigrations(the table has to exist first) and gated behind the existingusePostgrescheck — a complete no-op on SQLite, which has no autovacuum concept at all. Best-effort: a failed tune logs a warning and continues rather than blocking boot, since this is an optimization, never a correctness dependency.src/github/backfill.ts: documented, in place atrecordGitHubResponse(the single call site ofrecordGitHubRateLimitObservation), the decision not to implement write batching — see Correctness notes below.Correctness notes — why batching was evaluated and not implemented
The issue explicitly permits "either an implemented batching layer, or a documented decision that the current per-call INSERT pattern is acceptable... with the reasoning written down." I chose the latter:
QUEUE_CONCURRENCY's small worker-pool size) — nowhere near a volume where single-row Postgres INSERTs meaningfully pressure a connection pool.shouldWaitForGitHubRateLimit(src/github/rate-limit.ts) reads the latest row from this exact table for admission control across every self-host queue worker — including in a multi-instance/shared-Postgres deployment, where a buffering instance's writes would go stale to every other instance's reads, not just its own.Verification
Spun up a real Postgres 16 container (
docker run postgres:16) and ran the fulltest/integration/selfhost-pg.test.tssuite against it (normally CI-skipped, gated onPG_TEST_URL) — all 5 tests pass, including a new one asserting the exactpg_class.reloptionsvalues after applying the tuning twice (idempotency).Tests
test/unit/selfhost-pg-adapter-autovacuum.test.ts(new): the SQL constant targets the right table with a scale factor below Postgres's 0.2 default and is a single non-destructiveALTER;tuneGithubRateLimitObservationsAutovacuumcallsdb.execwith the exact SQL, fails open (does not throw) on a rejecteddb.exec, logs the underlying error message, and handles a non-Errorrejection without throwing on.messageaccess. Mutation-tested: reverting the scale factor to Postgres's 0.2 default correctly failed the "below default" assertion.test/integration/selfhost-pg.test.ts: new real-Postgres test verifying the autovacuum storage parameters are actually set on the table and that a second apply is a genuine no-op (not an error).Validation
npm run typechecknpx vitest run test/unit/selfhost-pg-adapter-autovacuum.test.tsPG_TEST_URL=... npx vitest run test/integration/selfhost-pg.test.ts(real Postgres 16, Docker)npm run test:changednpm run test:coverage(unsharded, full suite —src/selfhost/pg-adapter.ts/src/server.tsare Codecov-ignored percodecov.yml, validated instead by the real-Postgres integration test above;src/github/backfill.ts's change is comment-only)npm run db:migrations:check(no-op — no schema/migration file changes; this is a runtime storage-parameter tune, not DDL that needs migration-ledger tracking)npm run test:ci(the full local gate, exit 0)npm audit --audit-level=moderategit diff --checkScope
site/,CNAME,**/lovable/**, orCHANGELOG.mdSafety
usePostgres, never reached otherwise); no regression to rate-limit admission-control accuracy (no batching was introduced — the write path is unchanged, only a storage parameter on the target table)