Pool Postgres connections #3043

itowlson · 2025-03-10T23:33:47Z

~~Draft for now. Needs more testing and some tidying but I want to see if tests pass, and to verify with factors experts if I have the pools in the right place (AppState).~~

itowlson · 2025-07-14T22:19:16Z

See #3030 for local performance characteristics.

itowlson · 2025-07-31T00:33:46Z

There are now conformance tests for Postgres which will make it easier to regression-test this, but the conformance test PR is probably blocked on the data types PR, which is probably blocked on the conformance test PR.

It would be good to get some reviews on this little knot of PRs and figure out how to land them.

lann · 2025-08-01T13:11:32Z

crates/factor-outbound-pg/src/client.rs

-    async fn build_client(address: &str) -> Result<Self>
-    where
-        Self: Sized;
+pub trait ClientFactory: Send + Sync {


I think it should be safe to add + 'static here, which would let us remove all the related Send + Sync + 'static constraints elsewhere.

lann · 2025-08-01T13:28:25Z

crates/factor-outbound-pg/src/lib.rs

    type RuntimeConfig = ();
-    type AppState = ();
-    type InstanceBuilder = InstanceState<C>;
+    type AppState = Arc<RwLock<CF>>;


I'd recommend leaving interior mutability up to the trait implementation if possible; some implementations might not need to lock.

Suggested change

type AppState = Arc<RwLock<CF>>;

type AppState = Arc<CF>;

This would require changing the trait to use immutable borrows which is fine, but it will complicate the implementation of the ClientFactory.

I have deleted several stream-of-consciousness comments about this to summarize here:

There is a performance hazard with the current impl: this lock is held for the duration of the async call to ClientFactory::build_client. If that async call doesn't complete immediately then calls to open_connection from other instances (of the same app) will be blocked.

I don't know how much this problem would apply to PooledTokioClientFactory; that would depend on the runtime behavior of Pool::get.

Pushing interior mutability down into the ClientFactory won't fix this on its own but it would allow a fancier impl like using a read lock to check for the entry before falling back to the write lock. Or using moka.

moka worked really nicely. Thanks for the pointer!

crates/factor-outbound-pg/src/client.rs

rylev · 2025-08-01T13:38:42Z

crates/factor-outbound-pg/src/client.rs

-    async fn build_client(address: &str) -> Result<Self>
-    where
-        Self: Sized;
+pub trait ClientFactory: Send + Sync {


I know it didn't have doc comments before, but it would be awesome if it gained some.

rylev · 2025-08-01T13:41:51Z

crates/factor-outbound-pg/src/lib.rs

    type RuntimeConfig = ();
-    type AppState = ();
-    type InstanceBuilder = InstanceState<C>;
+    type AppState = Arc<RwLock<CF>>;


This would require changing the trait to use immutable borrows which is fine, but it will complicate the implementation of the ClientFactory.

crates/factor-outbound-pg/src/client.rs

Signed-off-by: itowlson <ivan.towlson@fermyon.com>

itowlson · 2025-08-03T23:59:11Z

crates/factor-outbound-pg/src/client.rs

+impl Default for PooledTokioClientFactory {
+    fn default() -> Self {
+        Self {
+            pools: moka::sync::Cache::new(CONNECTION_POOL_CACHE_CAPACITY),


@lann I'd value your input on whether we need to provide a way for hosts (e.g. multi-tenant environments) to configure cache (and pool) sizes and policies (e.g. idle time eviction). But also happy to punt that to a separate PR if that's not yet obvious.

I think punting on configurability is fine.

lann · 2025-08-04T14:23:20Z

crates/factor-outbound-pg/src/host.rs


-impl<C: Send + Sync + Client> spin_world::spin::postgres::postgres::HostConnection
-    for InstanceState<C>
+impl<CF: ClientFactory + Send + Sync> spin_world::spin::postgres::postgres::HostConnection


nit: Some redundant generic constraints remain in this file.

lann · 2025-08-04T14:28:34Z

crates/factor-outbound-pg/src/client.rs

+/// Max connections in a given address' connection pool
+const CONNECTION_POOL_SIZE: usize = 64;
+/// Max addresses for which to keep pools in cache.
+const CONNECTION_POOL_CACHE_CAPACITY: u64 = 1024;


I think we should start with a smaller number here. I think its more likely for a large number of idle connections to cause problems than for someone to actually want this many connections open for one app, and you'd almost certainly want this to be configurable at that point anyway.

Picking a differently arbitrary number... 16?

Signed-off-by: itowlson <ivan.towlson@fermyon.com>

itowlson force-pushed the pg-connection-pool branch 2 times, most recently from 7dca161 to 9f9bdce Compare March 18, 2025 01:09

itowlson force-pushed the pg-connection-pool branch 2 times, most recently from 7cfa3c2 to edc36eb Compare July 14, 2025 22:17

itowlson marked this pull request as ready for review July 14, 2025 22:18

itowlson requested review from kate-goldenring and lann July 14, 2025 22:18

lann reviewed Aug 1, 2025

View reviewed changes

crates/factor-outbound-pg/src/client.rs Outdated Show resolved Hide resolved

lann reviewed Aug 1, 2025

View reviewed changes

crates/factor-outbound-pg/src/client.rs Outdated Show resolved Hide resolved

rylev reviewed Aug 1, 2025

View reviewed changes

lann reviewed Aug 1, 2025

View reviewed changes

crates/factor-outbound-pg/src/client.rs Outdated Show resolved Hide resolved

lann reviewed Aug 1, 2025

View reviewed changes

crates/factor-outbound-pg/src/client.rs Outdated Show resolved Hide resolved

lann reviewed Aug 1, 2025

View reviewed changes

crates/factor-outbound-pg/src/client.rs Outdated Show resolved Hide resolved

Pool Postgres connections

08fecec

Signed-off-by: itowlson <ivan.towlson@fermyon.com>

itowlson force-pushed the pg-connection-pool branch from edc36eb to d951aae Compare August 3, 2025 23:52

itowlson requested a review from lann August 3, 2025 23:54

itowlson commented Aug 3, 2025

View reviewed changes

lann reviewed Aug 4, 2025

View reviewed changes

Use moka for caching pools

58b68da

Signed-off-by: itowlson <ivan.towlson@fermyon.com>

itowlson force-pushed the pg-connection-pool branch from d951aae to 58b68da Compare August 4, 2025 20:39

itowlson requested a review from lann August 4, 2025 20:40

lann approved these changes Aug 4, 2025

View reviewed changes

itowlson enabled auto-merge August 4, 2025 20:58

itowlson merged commit 0a26f5d into spinframework:main Aug 4, 2025
17 checks passed

macolso added the spin-3.4 Label to track items to land in the release of Spin v3.4 label Aug 5, 2025

macolso added this to Spin Triage Aug 5, 2025

macolso moved this to Done in Spin Triage Aug 5, 2025

rancherbot mentioned this pull request Aug 27, 2025

rddepman: bump spinCLI from 3.3.1 to 3.4.0 rancher-sandbox/rancher-desktop#9163

Merged

Pool Postgres connections #3043

Pool Postgres connections #3043

Conversation

itowlson commented Mar 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

itowlson commented Jul 14, 2025

Uh oh!

itowlson commented Jul 31, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lann Aug 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lann Aug 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

itowlson commented Mar 10, 2025 •

edited

Loading

lann Aug 1, 2025 •

edited

Loading

lann Aug 4, 2025 •

edited

Loading