Skip to content

fix(core): teach connection pools to pool their resources - #774

Open
cds-amal wants to merge 5 commits into
solana-foundation:mainfrom
cds-rs:fix/thread-thrash
Open

fix(core): teach connection pools to pool their resources#774
cds-amal wants to merge 5 commits into
solana-foundation:mainfrom
cds-rs:fix/thread-thrash

Conversation

@cds-amal

Copy link
Copy Markdown
Contributor

Following #759, this PR addresses r2d2 defaults that cause Surfpool's connection pools to allocate resources ahead of need:

  • Share one process-wide r2d2 scheduler instead of three threads per pool.
  • Start SQLite pools at one connection instead of ten, growing on demand.
  • Apply busy_timeout before other connection pragmas so lazy pool growth can tolerate lock contention.

Pooling behavior, connection limits, and lifetimes are unchanged.

Threads

r2d2 creates a private three-thread scheduler for every pool unless one is supplied. Pools live one per surfnet (SQLite, since #759) or one per database URL (Postgres), so the cost scales with live pools.

One process-wide scheduler now serves all pools: 3 threads per process instead of 3 per pool.

This is safe across pool lifetimes because r2d2 reap jobs hold a Weak reference to their pool; jobs for dropped pools retire as no-ops.

Connections

r2d2 defaults min_idle to max_size, so each SQLite pool opened all ten connections at surfnet startup, before any query ran.

SQLite now matches Postgres:

  • min_idle = 1
  • max_size = 10
  • grow on demand

SQLite Pragma order

Lazy growth exposed a connection-setup race.

busy_timeout was the last SQLite pragma applied during connection setup, so earlier statements ran with SQLite's default timeout of zero. If another connection held a lock, setup failed immediately and r2d2 discarded and replaced the connection.

On main this is latent because connections are created before startup begins writing. With lazy growth, connection setup can interleave with those writes.

busy_timeout now runs first, so setup waits out contention like later traffic does.

Validation

The branch includes temporary lifecycle counters and an ignored census workload; the final commit removes them.

Run the census with:

cargo test -p surfpool-core --lib storage::census -- \
           --ignored --nocapture --test-threads=1

For ten on-disk surfnets running zero queries:

commit platform opens, sequential ×10 live connections, 10 held setup errors
census (main + counters) both 100 100 0
min_idle macOS 32 to 40 31 to 38 ~25 logged, ~15 discarded
min_idle Linux 20 to 21 20 0 to 2 logged, 0 to 1 discarded
busy_timeout (tip) both 20 20 0

The endpoints are deterministic across macOS and Linux. The intermediate commit varies because it exposes the setup race; moving busy_timeout first closes it on both.

Dropping the ten held surfnets returns all counters to zero, preserving the connection-release behavior established by #759.

For threads, three full core-suite runs (~700 tests, Linux, 16 cores) peaked at exactly 3 r2d2-worker threads for the process. Main pays three per live pool; ten held surfnets therefore require thirty.

Historical context

Before #759, isolated in-memory pools were cached for the process lifetime. A test binary was observed at 8,316 threads against macOS's 9,216 per-process limit, with 96% named r2d2-worker.

#759 removed the cache. This PR removes the remaining per-pool resource multiplier.

- Count pools and SQLite connections: created, dropped, live, and peak.
- Instrument r2d2 directly so deltas are exact, assertable, and
  portable.
- Add an ignored census workload covering sequential, concurrent, and
  drop costs.
- Baseline: 10 idle surfnets open 100 connections and keep 10 pools,
  each with a private 3-thread scheduler.
- Census is temporary branch instrumentation and is removed by the final
  commit.

Run alone because counters are process-global:

    cargo test -p surfpool-core --lib storage::census -- \
        --ignored --nocapture --test-threads=1
- Share one process-wide r2d2 scheduler across SQLite and Postgres
  pools.
- Remove the default 3-thread scheduler cost per pool.
- Preserve pooling behavior, limits, and connection lifetimes.
- Dropped pools are not retained; r2d2 reap jobs hold only weak
  references.
- Before solana-foundation#759: up to 8,316 threads, 96% `r2d2-worker`.
- After: 3 `r2d2-worker` threads across the full core test suite.
- Set SQLite `min_idle` to 1, matching Postgres.
- Avoid opening 10 connections per surfnet before any query runs.
- Keep `max_size` at 10 and grow on demand.
- Apply `busy_timeout` before setup can encounter SQLite locks.
- Prevent on-demand pool growth from discarding connections on transient
  contention.
- Eliminate observed `database is locked` churn.
- Census: 53 opens / 36 survivors -> 20 opens / 20 survivors on macOS,
  with the same clean result on Linux.
- Remove the temporary instrumentation used to measure this branch.
- Keep the before/after measurements reproducible from earlier commits.
- Restore the plain Diesel connection manager.
@cds-amal
cds-amal marked this pull request as ready for review August 23, 2026 01:36
@greptile-apps

greptile-apps Bot commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR reduces database-pool resource usage by sharing one process-wide r2d2 scheduler, lazily growing SQLite pools from one connection, and applying SQLite’s busy timeout before the remaining connection pragmas.

  • Adds a shared three-thread scheduler for SQLite and PostgreSQL pools.
  • Sets SQLite min_idle to one while preserving its ten-connection maximum.
  • Reorders SQLite connection pragmas so lock contention is handled during setup.
  • Adds scheduled-thread-pool as a direct core dependency.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
crates/core/src/storage/mod.rs Introduces a lazily initialized process-wide scheduler shared by enabled SQL pool implementations.
crates/core/src/storage/postgres.rs Configures existing per-URL PostgreSQL pools to use the shared scheduler without changing their size limits.
crates/core/src/storage/sqlite.rs Configures per-Surfnet SQLite pools for lazy growth and moves busy-timeout setup ahead of other connection pragmas.
crates/core/Cargo.toml Adds the scheduler crate as an explicit dependency required by the shared pool scheduler.
Cargo.lock Records the existing scheduler package as a direct dependency of surfpool-core.

Reviews (2): Last reviewed commit: "test(core): remove the storage census" | Re-trigger Greptile

@cds-amal
cds-amal marked this pull request as draft August 23, 2026 01:48
@cds-amal
cds-amal marked this pull request as ready for review August 25, 2026 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant