feat: configurable pool size (poolMaxSize setting) - #63
Open
aesslinger wants to merge 1 commit into
Open
Conversation
Port the built-in postgres driver's configurable connection-pool size (tabularis#681) to the plugin, and close a latent parity gap in the same code path the 82-test suite doesn't cover (it compares query results, not connection counts). Previously build_pool never set max_size, so deadpool fell back to PoolConfig::default() = logical_cores × 2 (up to ~32 on a 16-thread machine) — up to ~3× more backend connections per target than the built-in's pinned 10, the wrong default for the pgBouncer use case this setting exists for. Changes: - .tabularium: declare a poolMaxSize number setting (default 10), matching the host's PluginSettingDefinition serde contract - src/settings.rs (new): port postgres_pool_max_size_from_value verbatim from pool_manager.rs (u64/i64/string parse, zero/invalid -> default 10, cap 64) into a process-global Mutex<usize>; default initialized to 10 so pools are correctly sized even if initialize never arrives - src/handlers/connection.rs: initialize no longer discards params — reads params['settings']['poolMaxSize'] and stores it; never panics (host silently ignores initialize errors) - src/client.rs: build_pool applies .max_size(settings::pool_max_size()) in both TLS and non-TLS branches, restoring parity with the built-in's default of 10 The one intentional adaptation: the built-in reads the setting from the host config cache on each pool build; an external plugin has no access to that cache, so the value is captured at the initialize RPC. This is the adaptation the issue author prescribed (tabularis#681 comment). Mutex (most-recent-initialize-wins) mirrors the built-in's read-current- config intent as closely as an external plugin can. Defense in depth: the 64 cap is the single bound on pool size regardless of what arrives via initialize; zero falls back to 10 (never a dead 0-connection pool); every parse path is infallible (null/bool/object/ array/negative/garbage-string all -> default). Tests: 16 settings tests (4 builtin parity cases ported verbatim + edge cases + initialize-handshake path) and 3 build_pool tests asserting pool.status().max_size directly (default 10 not cpu×2, custom honored, oversized clamped) — the connection-count coverage the parity suite lacks. Shared tokio::sync::Mutex serializes all tests touching the global. Verified: 133/133 unit tests (deterministic across 8 runs), clippy -D warnings clean, fmt clean, 8/8 live_db against podman pg-tabularis-test. Closes #61
Version suggestionBased on this PR's title (
This is informational only — no tag or release is created automatically yet. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Ports the built-in
postgresdriver's configurable connection-pool size (tabularis#681) to the plugin, and closes a latent parity gap in the same code path. Closes #61.The parity gap this fixes
build_poolpreviously never setmax_size, so deadpool fell back toPoolConfig::default()=logical_cores × 2(up to ~32 on a 16-thread machine) — up to ~3× more backend connections per target than the built-in's pinned 10. The 82-test parity suite compares query results, not connection counts, so it never caught this. For the pgBouncer use case this setting exists for (a small client pool), the plugin was worse out of the box.What changed
.tabularium— declare apoolMaxSizenumber setting (default 10), matching the host'sPluginSettingDefinitionserde contract (key/label/type/default/description).src/settings.rs(new) — portpostgres_pool_max_size_from_valueverbatim from the built-in'spool_manager.rs(u64/i64/string parse, zero/invalid → default 10, cap 64) into a process-globalMutex<usize>, default-initialized to 10 so pools are correctly sized even ifinitializenever arrives.src/handlers/connection.rs—initializeno longer discards params: readsparams["settings"]["poolMaxSize"]and stores it. Never panics (the host silently ignoresinitializeerrors).src/client.rs—build_poolapplies.max_size(settings::pool_max_size())in both TLS and non-TLS branches, restoring parity with the built-in's default of 10.The one intentional adaptation (not a gap)
The built-in reads the setting from the host config cache on every pool build (
get_cached_config().plugins.get("postgres")...). An external plugin has no access to that cache — it receives settings only via theinitializeRPC — so the value is captured atinitializeinto a process-global. This is the exact adaptation the issue author prescribed (see the tabularis#681 comment and #61's plan).Mutex(most-recent-initialize-wins) mirrors the built-in's read-current-config intent as closely as an external plugin can; a corrected re-init replaces the value, matching a config-cache re-read.Feature-parity verification against tabularis#681
Compared the current full PR #681 diff (1 commit, CLEAN), review, and all 15 changed files against this work:
postgres_pool_max_size_from_valuepool_max_size_from_value.max_size(postgres_pool_max_size()).max_size(settings::pool_max_size())key/label/type/default/descriptionFiles with no plugin equivalent (correctly N/A):
useDrivers.ts(external plugins have no TS fallback — manifest is source of truth) and the 11 i18n locale files (the host resolves labels underbuiltin.postgres.poolMaxSize, but this plugin's id ispostgresql, so those keys never match and labels fall back to the manifest's English — the issue itself flags this as non-blocking).Defense in depth
initialize(10_000, a poisoned payload, a re-init),pool_max_size()can never exceed 64. Clamp happens before storage.poolMaxSize: 0falls back to 10, so a pgBouncer user who fat-fingers 0 gets a working 10-connection pool, not a dead 0-connection pool.initializeerrors, so a panic would kill the handshake. Every parse path is infallible: null/bool/object/array/negative/garbage-string all → default.initializenever arrives or is dropped, the global stays at default 10.Verification
-D warningsclean (resolved anawait_holding_locklint viatokio::sync::Mutexfor the test lock)build_pool), deterministic across 8 consecutive runspg-tabularis-test): 8/8 — pool connects, startup scripts run, SSL mode resolves, queries round-trip with the newmax_sizewiring in placeVersioning
feature:type → minor version impact per the README's type→bump table. Labeledprerelease:beta(current channel).Closes #61.