feat: Add reuse_connections profile option#670
Conversation
|
Hi @dlouseiro ! Thanks for the PR. I'm checking it internally to see which alternatives we can have to these problems, but tbh seems like your's is the way to go. I'm a bit worried with the overhead of having to recreate the whole client per each node. Have you already tested this in prod? How is it going? |
|
Apart from what I mentioned, would you check if you can add to this PR:
With these, the recreation of the client should be way faster. Not sure if we can do something else to improve the performance, feel free to suggest other things Thanks! |
Adds the `reuse_connections` option (default `True`) to the dbt profile.yml configuration table. When set to `False`, dbt closes the underlying connection at the end of each model so the next model opens a fresh TCP connection — useful on multi-replica ClickHouse Cloud services where the load balancer routes by TCP connection and a long-lived pool would otherwise pin a `dbt run` to a single replica. Tracks ClickHouse/dbt-clickhouse#669 / ClickHouse/dbt-clickhouse#670.
Thank you @koletzilla . I'll have a look at your suggestions. |
Of course, this is mostly a proposal of a solution for the issue I created. Might not be the best, but as it was not too much code I decided to add it. We did not yet test this in prod as we'd ideally have this released on the main distribution of |
Adds a profile-level boolean `reuse_connections` (default `true`, preserving current behaviour). When set to `false`, dbt closes the underlying connection at the end of each model so the next model opens a fresh TCP connection — useful for multi-replica ClickHouse Cloud services where the load balancer routes by connection and a long-lived pool would otherwise pin a `dbt run` to a single replica. Mirrors the equivalent option on dbt-snowflake. The change gates the existing no-op `release()` on the new flag and falls through to the base `SQLConnectionManager.release()` (which calls `self.close(conn)`, which calls `connection.handle.close()` on the clickhouse-connect / clickhouse-driver handle). Closes ClickHouse#669
Addresses review on ClickHouse#670: with reuse_connections=false a client is created per model, so the per-connection setup work now matters much more. - Cache the EXISTS DATABASE probe process-wide (invalidated by database_dropped) - Drop the lightweight-delete capability probe; lw deletes are GA on all supported ClickHouse versions (min 23.3 << CI min 25.3) - Apply allow_nondeterministic_mutations via connection settings instead of a per-client query + SET Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
bb2e9d2 to
03f710d
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
Reviewed by Cursor Bugbot for commit 03f710d. Configure here.
@koletzilla I pushed a new commit to tackle your comments above. Let me know what you think! |
…lw_deletes has_lw_deletes is now always True (lw deletes are GA on all supported ClickHouse versions), so the previous guard never fired. Checking use_lw_deletes (the profile opt-in) restores the intended behaviour: delete_insert and microbatch require use_lw_deletes: true, which also ensures allow_nondeterministic_mutations is injected — necessary for DELETE with subquery predicates on replicated tables. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…calls Mirrors the pattern in test_exchange_check.py where _exchange_result is reset by an autouse fixture. Removes the manual .clear() calls inside individual tests so future cache tests can't accidentally skip cleanup. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…y guard
- test_connections: assert reuse_connections defaults to True
- test_dbclient: add symmetric case for use_lw_deletes=False (has_lw_deletes
remains True, use_lw_deletes=False)
- test_incremental_strategy (new): nine tests for validate_incremental_strategy
covering the bug fix — delete_insert/microbatch raise when use_lw_deletes=False,
pass when True, and other strategies are unaffected

Summary
Resolves #669.
Adds a profile-level boolean
reuse_connections(defaulttrue, preserving current behaviour). When set tofalse, dbt closes the underlying connection at the end of each model so the next model opens a fresh TCP connection — useful for multi-replica ClickHouse Cloud services where the load balancer routes by connection and a long-lived pool would otherwise pin adbt runto a single replica.Mirrors the equivalent option on dbt-snowflake. The change gates the existing no-op
release()on the new flag — whenfalse, it falls through to the baseSQLConnectionManager.release(), which callsself.close(conn).Performance: reduced per-model connection startup overhead
With
reuse_connections: falsea client is created per model, so per-connection setup cost matters much more. Three optimisations were applied:_ensure_databasecached process-wide — theEXISTS DATABASEprobe now runs at most once per process (invalidated bydatabase_dropped), instead of once per client creation.has_lw_deletesis alwaysTrueand no server round-trip is needed.allow_nondeterministic_mutationsvia connection settings — applied at client construction via_conn_settingswhenuse_lw_deletes: true, instead of a per-clientSETquery.Bug fix:
delete_insert/microbatchguardThe
validate_incremental_strategyguard was checkinghas_lw_deletes(server capability, now alwaysTrue) instead ofuse_lw_deletes(profile opt-in). This meant a model withincremental_strategy: delete+insertcould silently bypass validation when the profile haduse_lw_deletes: false, and then fail at runtime on replicated tables becauseallow_nondeterministic_mutationswas not injected. Fixed to checkuse_lw_deletes.Verified
release()behaviour, ND mutation settings, DB existence caching.reuse_connections: false.Checklist
Note
Medium Risk
Changes connection lifecycle and removes runtime lightweight-delete capability probing, which could affect edge-case servers or misconfigured profiles; incremental validation behavior is stricter for delete_insert/microbatch without use_lw_deletes.
Overview
Adds profile flag
reuse_connections(defaulttrue). Whenfalse,ClickHouseConnectionManager.release()delegates to the SQL base class so connections close after each model and new ones can land on different ClickHouse Cloud replicas behind sticky load balancing.To keep per-model reconnects cheap,
EXISTS DATABASEis cached process-wide (invalidated ondatabase_dropped), lightweight-delete server probes and runtimeSETqueries are removed (lw deletes treated as always available on supported versions), andallow_nondeterministic_mutationsis injected via connection settings whenuse_lw_deletes: true.Bug fix:
validate_incremental_strategynow gatesdelete_insert/microbatchonuse_lw_deletesinstead ofhas_lw_deletes, so models cannot pass validation without the profile opt-in. Related lw-delete error strings were removed fromerrors.py.Reviewed by Cursor Bugbot for commit d627613. Bugbot is set up for automated code reviews on this repo. Configure here.