feat: allow passing a startup script per connection#352
Conversation
Run optional SQL on every new pooled connection (MySQL/SQLite after_connect, Postgres deadpool post_create) so session settings like set_config apply to every query. Editable via a new "Advanced" tab in the connection editor. Closes TabularisDB#350
Code Review SummaryStatus: No Issues Found | Recommendation: Merge All feedback from the previous review round has been addressed in this incremental update. Files Reviewed (10 files)
Previous Review Summaries (5 snapshots, latest commit 50e9ae6)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 50e9ae6)Status: No Issues Found | Recommendation: Merge Overview
Files Reviewed (3 files)
Previous Issue Resolution
Previous review (commit 947325b)Status: No Issues Found | Recommendation: Merge Overview
Files Reviewed (3 files)
Previous Issue Resolution
Previous review (commit 2186106)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (3 files)
Fix these issues in Kilo Cloud Previous review (commit 30b5548)Status: No Issues Found | Recommendation: Merge The previous CRITICAL issue has been resolved. Changes verified
Files Reviewed (2 incremental files)
Previous review (commit 14eeac9)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
Files Reviewed (15 files)
Reviewed by kimi-k2.7-code-20260612 · Input: 82K · Output: 10.9K · Cached: 1M |
build_connection_key hashed only connection_id/host/tls, so editing a saved connection's startup script reused the cached pool and the new script never ran until app restart. Fold a SHA-256 of the trimmed script into the key (only when set, so script-free pools keep their keys). Addresses the critical review finding on TabularisDB#352.
|
Good catch — fixed in 30b5548.
Added two unit tests: the key changes when the script changes, and a blank/whitespace script is treated as absent (no key fragmentation). |
|
Thanks for this PR. Couple of things before this goes in:
|
A broken startup script now fails fast with a clearly attributed error instead of a misleading pool timeout or generic connection error: - MySQL/SQLite preflight the script on a throwaway connection. sqlx swallows after_connect errors and retries until the acquire timeout, reporting "pool timed out"; the preflight surfaces "Startup script failed: ..." up front. Connection failures are returned verbatim so a bad host/credentials is not mislabelled as a script error. - Postgres post_create reports the startup script as the cause. Also swap the Advanced-tab startup-script textarea for the shared SqlEditorWrapper (Monaco) for syntax highlighting, theming and font settings, matching the editor and other SQL fields.
|
Both addressed in 2186106. 1. Error story. Kept it hard-blocking — for the RLS-bypass case a half-configured session silently serving queries is worse than a clear failure — but made the failure fast and clearly attributed, and distinct from a connectivity error. Worth flagging what I found while doing this: for MySQL/SQLite the old behaviour wasn't actually "bubbles straight out". sqlx swallows
One residual edge: a script that passes preflight but fails non-deterministically on a later pooled connection still hits sqlx's retry → 2. Editor. Swapped the textarea for The SQLite integration test now asserts the error is attributed ( |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
…meout The MySQL preflight was wrapped in the connect timeout but SQLite's was not. SQLite is file-based so a hang is unlikely, but a custom VFS or a path on a stalled network mount could wedge pool creation indefinitely. Wrap it in a 30s timeout to match MySQL.
Tabs no longer squish when many are present (network drivers show up to 7). The strip now scrolls horizontally with edge arrows that appear only when there is overflow in that direction, a fading mask at the edges, hidden scrollbar, and auto-scroll of the active tab into view.
NewtTheWolf
left a comment
NewtTheWolf
left a comment
There was a problem hiding this comment.
Thanks for this — really clean work! 🙏 Pool-key fold for cache invalidation, error attribution, and the preflight-vs-post_create asymmetry are all well done. Tested locally against a Postgres RLS fixture (restricted role + app.bypass_rls policy): 2 → 4 → 2 rows across the pool, broken script → attributed error, 5/5 Rust tests green. Three blockers, one nit below.
| connect_timeout.as_millis() | ||
| ) | ||
| })??; | ||
| pool_options = pool_options.after_connect(move |conn, _meta| { |
There was a problem hiding this comment.
blocker — the script runs twice on the first connection: once in the preflight throwaway connection above, then again here in after_connect. A side-effecting script (INSERT, counters, …) firing an extra time on pool creation is a real footgun. Please make the preflight validation-only (don't execute side effects) so the script runs exactly once per connection.
- Make preflight validation-only (rolled-back transaction) so side-effecting scripts run exactly once per connection via the per-connection hooks - Add symmetric PostgreSQL preflight with clean error attribution (was surfacing deadpool's raw PoolError debug struct on first use) - Switch tab-fade useEffect to useLayoutEffect to avoid one-frame flash (react.md rule TabularisDB#2) - i18n hardcoded scroll-tabs aria-labels across all 8 locales
NewtTheWolf
left a comment
NewtTheWolf
left a comment
There was a problem hiding this comment.
All four points addressed cleanly — thank you! 🙏 The transaction-rollback preflight is a neat way to keep the script side-effect-free until the per-connection hook, and giving Postgres a symmetric preflight kills the deadpool debug leak. Verified locally: builds, 5/5 Rust tests green.
One tiny FYI (non-blocking): the preflight validates inside a transaction, so a non-transactional statement (VACUUM, CREATE INDEX CONCURRENTLY, …) would be rejected here even though it would run fine in the real post_create/after_connect hook — and MySQL implicitly commits DDL, so a DDL script would still apply once despite the rollback. Both are irrelevant for the intended SET/set_config use; just worth a one-line code comment if you want. LGTM either way.
True, but it's hard for me to come up with a reason to do that. |
|
It looks like something non-blocking. |
3 participants
Closes #350.
Adds an optional startup script to a connection — SQL run on every new pooled connection, so session settings like
SELECT set_config('app.bypass_rls', 'on', false);(or anySET) apply to every query regardless of which pooled connection serves it. This is the DataGrip-style "startup script" requested in the issue, and makes RLS-bypass-in-dev practical.Changes
ConnectionParams.startup_script, persisted with the connection (non-secret).after_connect, Postgres via deadpoolpost_create. Blank/whitespace scripts are skipped; multiple statements are separated by;.Tests
SQLite pool tests (no DB server needed) cover: script runs on connect, blank script is skipped, invalid script surfaces an error.