Skip to content

Fix PostgreSQL pool size setting for pgBouncer - #681

Open
DhruvShah-Dev wants to merge 1 commit into
TabularisDB:mainfrom
DhruvShah-Dev:fix/postgres-pool-size-setting
Open

Fix PostgreSQL pool size setting for pgBouncer#681
DhruvShah-Dev wants to merge 1 commit into
TabularisDB:mainfrom
DhruvShah-Dev:fix/postgres-pool-size-setting

Conversation

@DhruvShah-Dev

Copy link
Copy Markdown
Contributor

Summary

  • add a PostgreSQL poolMaxSize built-in driver setting
  • use the setting when creating the PostgreSQL connection pool, defaulting to the existing size of 10
  • clamp invalid, zero, or oversized values and cover parsing edge cases

Validation

  • pnpm.cmd typecheck
  • pnpm.cmd lint
  • pnpm.cmd exec vitest run tests/utils/driverUI.test.tsx tests/i18n/registryLocale.test.ts
  • cargo test postgres_pool_max_size --lib --no-run

Refs #71

@kilo-code-bot

kilo-code-bot Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Files Reviewed (15 files)
  • src-tauri/src/drivers/postgres/mod.rs
  • src-tauri/src/pool_manager.rs
  • src-tauri/src/pool_manager_tests.rs
  • src/hooks/useDrivers.ts
  • src/i18n/locales/de.json
  • src/i18n/locales/en.json
  • src/i18n/locales/es.json
  • src/i18n/locales/fr.json
  • src/i18n/locales/it.json
  • src/i18n/locales/ja.json
  • src/i18n/locales/ko.json
  • src/i18n/locales/pt-BR.json
  • src/i18n/locales/ru.json
  • src/i18n/locales/tl.json
  • src/i18n/locales/zh.json

The new poolMaxSize PostgreSQL setting mirrors the existing MySQL built-in setting pattern precisely:

  • postgres_setting_value / postgres_pool_max_size_from_value in pool_manager.rs reuse the same parsing approach as the MySQL helpers, with correct edge-case handling (zero, negatives, non-numeric strings, oversized values all fall back to the default of 10 or clamp to the 64 cap).
  • The pool builder now calls postgres_pool_size() instead of the hard-coded 10.
  • The manifest (postgres/mod.rs) and TS fallback (useDrivers.ts) declare the setting consistently, and the default value is sourced from the shared DEFAULT_POSTGRES_POOL_MAX_SIZE constant on the Rust side.
  • Tests cover the no-setting, numeric, string, invalid, zero, and oversized cases.
  • i18n keys are added at the correct nesting level (pluginSettings.builtin.postgres.poolMaxSize) across all locales, matching the MySQL sibling structure.

No security, runtime, or logic issues identified in the changed lines.


Reviewed by glm-5.2 · Input: 32.2K · Output: 4.4K · Cached: 304.2K

@aesslinger

Copy link
Copy Markdown
Contributor

Thanks for this — the pgBouncer case is real and the clamp/parse logic is clean. Tracking the plugin-side follow-up here so it's on record: I've opened tabularis-postgresql-plugin#61 to port this capability to the extracted PostgreSQL driver plugin (the repo that's meant to replace this built-in postgres driver).

Two things worth flagging from the plugin's perspective:

1. Pre-existing latent parity gap in the same code path. The plugin's build_pool never sets max_size at all, so deadpool falls back to PoolConfig::default() = logical_cores × 2 (deadpool-0.12.3/src/util.rs:10) — 32 on a 16-thread machine, vs. this driver's hardcoded 10. The 82-test parity suite compares query results, not connection counts, so it doesn't catch this. Porting this PR's default of 10 (rather than cpu×2) closes that gap as a side effect.

2. The host already passes settings to external plugins, but the plugin currently discards them — its initialize handler is a no-op that ignores params (src/handlers/connection.rs:10), and its .tabularium manifest declares no settings array. So the plugin-side work is: declare poolMaxSize in the manifest, make initialize read and store it in a process-global, and apply it via cfg.pool = Some(PoolConfig { max_size, .. }) in build_pool. The parse/clamp logic from postgres_pool_max_size_from_value will be ported verbatim for parity.

One minor note on i18n: PluginSettingsPage.tsx resolves setting labels via settings.plugins.pluginSettings.builtin.${pluginId}.${def.key}.label with defaultValue: def.label. The keys this PR adds are under postgres (the built-in id); the plugin's id is postgresql, so its labels will fall back to the English def.label from the manifest rather than the translated keys. The setting still works for plugin users — just untranslated unless/until parallel postgresql-keyed entries are added. Not a blocker for this PR; mentioning it so the i18n discrepancy is known.

Neither point blocks this PR — just recording the downstream impact and the plan to mirror it.

@aesslinger

Copy link
Copy Markdown
Contributor

Plugin-side follow-up is ready and waiting on this PR.

Opened tabularis-postgresql-plugin#63 to port this capability to the extracted PostgreSQL driver plugin. It's complete, parity-verified against this PR, and CI-green — ready to ship once this lands.

What it does, mirroring this PR:

  • Parser ported verbatim: pool_max_size_from_value in src/settings.rs is a byte-for-byte port of postgres_pool_max_size_from_value (same u64 → i64 → string chain, > 0 filter, .min(64), unwrap_or(10)), with the matching DEFAULT = 10 / MAX = 64 constants.
  • Setting declared in the manifest: .tabularium now declares the poolMaxSize number setting (default 10) with the same field values this PR adds to PostgresDriver::manifest, matching the host's PluginSettingDefinition serde contract.
  • initialize wired up: the plugin's initialize handler no longer discards params — it reads params["settings"]["poolMaxSize"] and stores it in a process-global, which build_pool applies via .max_size(...). This is the one necessary adaptation (an external plugin has no access to get_cached_config(), so the value is captured at the initialize RPC rather than re-read from the host config cache on each pool build) — exactly the approach outlined in the earlier comment here.
  • The latent parity gap is closed as a side effect: the plugin's build_pool previously never set max_size, so deadpool fell back to PoolConfig::default() = logical_cores × 2 (up to ~32 on a 16-thread machine) vs. this driver's pinned 10. Defaulting to 10 restores parity.

Verification on the plugin side: 133/133 unit tests (including the 4 cases from this PR ported verbatim, plus edge cases and 3 build_pool tests asserting pool.status().max_size directly — the connection-count coverage the 82-test parity suite lacks), clippy -D warnings clean, 8/8 live-db tests against a local Postgres. PR #63 is fully CI-green and mergeable; I'll land it once this merges.

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.

2 participants