Reconnect stale pooled connections via ping_before_query - #8
Merged
Conversation
Long-lived pooled connections get closed out from under the client by
ClickHouse's idle_connection_timeout, a load balancer recycling idle
sockets, or a server restart. The next use of such a socket hit
recv()==0 and raised ConnectionError("closed: ..."), which the pool's
single automatic retry only partially covered (bursts where several
pooled sockets die at once exhausted it).
Enable clickhouse-cpp's connection-resilience options in Client and
thread them through Pool, all overridable:
- ping_before_query (default true): ping and transparently reconnect a
dead socket before each query, so a stale connection is repaired
instead of surfacing an error
- tcp_keepalive (default true): OS-level TCP keepalive
- retry_timeout (default 1s): reconnect backoff, lowered from the
upstream 5s so recovery and give-up stay quick
Expose the three as Client accessors. Add specs for the defaults,
overrides, pool forwarding, and end-to-end recovery when the server
drops the connection (via an out-of-process TCP proxy, since
Client#initialize connects without releasing the GVL).
tycooon-review-bot
suggested changes
Jul 20, 2026
…sever Addresses the Codex review on #8. Session `settings:` were applied once via a `SET` in the pool's object factory, so a transparent ping_before_query reconnect — which reuses the same Client on a fresh socket, bypassing the factory — silently reverted to server defaults until the client was discarded. Apply them instead as per-query settings in the binding: they ride every query packet and thus survive any reconnect. Sent as important, so an unknown setting name still errors loudly rather than being ignored. Drop the pool's SET path and settings_sql. The proxy's sever_all only sent SIGUSR1 without waiting for the child's trap to close the tunnels, so a follow-up query could race the still-live socket (making the positive example skip the reconnect and the negative one flake). Add a pipe the trap writes after closing; sever_all blocks on it. Add a pool getSetting spec proving settings survive a severed-socket reconnect.
Addresses the Codex re-review of #8. The per-query settings from the previous commit only rode execute/query/query_value/query_each. Block inserts go through Client::Insert, which builds its own INSERT query internally and never consulted the client's settings — so Pool.new(settings: ...).insert silently dropped insert-time settings (insert_quorum, max_insert_threads, ...) that the README documents as belonging there. Add patch 0002-carry-settings-into-insert: it gives clickhouse-cpp's Client::Insert / Impl::Insert an optional settings parameter, applied as important per-query settings on the INSERT query it already generates (reusing all its column logic). The binding passes the client's default settings through. Update the README insert note and add a regression test asserting an unknown pool setting makes an insert fail — proving settings now reach inserts, not just queries.
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
2 participants
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.
Problem
Long-lived pooled connections get closed out from under the client — ClickHouse's
idle_connection_timeout, a load balancer recycling idle sockets, or a server restart. The next use of such a socket hitsrecv() == 0and raisesConnectionError: closed: .... (The errno in the message is a stale, meaningless value left in the thread —Success,Operation now in progress, etc. — all the samerecv()==0.) The pool's single automatic retry only partially covers this: a burst where several pooled sockets die at once exhausts it.Change
Enable clickhouse-cpp's built-in connection-resilience options in
Clientand thread them throughPool, all overridable:ping_before_querytruetcp_keepalivetrueretry_timeout1The three are exposed as
Clientaccessors. The pool's one-shotConnectionErrorretry stays as a backstop for the residual race (socket dies between the ping and the query).Note on defaults
ping_before_querydefaults on, which adds one round-trip per query — this is the behavior change that repairs stale connections out of the box. Setping_before_query: falseto opt out and manage staleness yourself.Tests
ping_before_query: false)The recovery tests drive an out-of-process TCP proxy that severs live tunnels on demand —
Client#initializeconnects without releasing the GVL, so an in-process proxy would deadlock.