persist: proactively recycle connections on draining CRDB nodes [POC] - #38097
Draft
jubrad wants to merge 2 commits into
Draft
persist: proactively recycle connections on draining CRDB nodes [POC]#38097jubrad wants to merge 2 commits into
jubrad wants to merge 2 commits into
Conversation
Upgrade deadpool 0.9.5 -> 0.12.3 and deadpool-postgres 0.10.3 -> 0.14.1 to get access to Pool::resize, and wire PostgresClient::get_connection to apply a changed connection_pool_max_size knob to the live pool on the next acquire. persist_consensus_connection_pool_max_size and pg_timestamp_oracle_connection_pool_max_size previously required a process restart to take effect, which made the pool cap unusable as an operational lever during CRDB maintenance or incidents. Note a monitoring behavior change that comes with the deadpool upgrade: the connpool_available metric is now a non-negative idle count. Acquires queued on an exhausted pool were previously visible as negative available and are no longer externally observable. The deadpool API migration itself: the Manager trait is natively async, recycle takes a Metrics argument, and pre_recycle hook errors are constructed with HookError::message. The TTL-culling semantics of the pre_recycle hook are unchanged. deadpool 0.12 requires lazy_static 1.5, which moves the duplicate-spin skip in deny.toml from 0.5.2 to 0.9.9. Adds a pool_resize_applies_on_acquire test in mz-postgres-client that verifies grow and shrink both apply on acquire. It opts in via the same MZ_PERSIST_EXTERNAL_STORAGE_TEST_POSTGRES_URL variable as the persist external-storage tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
jubrad
force-pushed
the
crdb-pool-drain-recycle
branch
5 times, most recently
from
August 7, 2026 17:59
1aeedad to
c74f117
Compare
When a CockroachDB node drains (rolling upgrade, maintenance), its share of the consensus pool's connections is closed by the server with no advance protocol-level notice: idle connections receive a bare TCP FIN and anything still open at the end of the drain is force-closed. The pool only discovers a dead connection when it is next checked out. Give the pool the earliest available signal instead. When the new persist_consensus_connection_pool_drain_aware_recycling dyncfg (default off) is enabled, every connection is stamped with crdb_internal.node_id() at creation, and a background watchdog polls crdb_internal.gossip_liveness every 5 seconds for current cluster members that are draining or decommissioning. The pre_recycle hook discards connections on such nodes, rate limited to one cull per 500ms so the pool migrates off the node gradually rather than ejecting every affected connection at once and stalling acquires behind a burst of re-establishment. The liveness poll joins against gossip_nodes to exclude liveness tombstones: records for long-decommissioned nodes linger indefinitely, frequently with draining still set. Liveness expiration is deliberately not consulted, since an expired record is a symptom (crash, partition, clock skew) that TCP keepalives already handle, and acting on it could mass-cull a healthy pool when gossip itself is stale. On non-CockroachDB backends the stamping and polling queries fail and are tolerated (the connection is left unstamped and the draining set stays empty), but the flag should be left off there to avoid the wasted round trip per connection creation. The CI default follows the metadata store: on for CockroachDB, off otherwise. Adds a test that marks a connection's node as draining and verifies the pooled connection is discarded and replaced on the next acquire, opted in via the same environment variable as the persist external-storage tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
jubrad
force-pushed
the
crdb-pool-drain-recycle
branch
from
August 7, 2026 18:24
c74f117 to
4b640f7
Compare
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.
Motivation
Stacked on #38050 (deadpool upgrade + live pool resize) — review that first; this diff includes it until it merges.
During a CRDB node drain, the server gives clients no protocol-level advance notice: new connections are refused with
AdminShutdown(57P01), idle connections are closed with a bare TCP FIN, and anything still open at the end of the drain is force-closed (seepkg/sql/pgwire/server.go,ErrDrainingNewConn/ErrDrainingExistingConn). Deadpool only discovers a dead connection at its next checkout, so during a rolling upgrade the pool sits on dead-connections-walking until first use.This PR gives the pool the earliest signal CRDB exposes — the gossiped
drainingflag — and recycles connections off a draining node during the drain grace window instead of after the close.Changes
crdb_internal.node_id()at creation (one extra round trip per create, gated).crdb_internal.gossip_livenessevery 5s for current cluster members that are draining or decommissioning (draining OR membership != 'active', joined againstgossip_nodesto exclude liveness tombstones of long-decommissioned nodes, which linger forever withdrainingstill set). Livenessexpirationis deliberately not consulted: an expired record is a symptom (crash, partition, clock skew) that TCP keepalives already handle, and acting on it could mass-cull a healthy pool when gossip itself is stale.pre_recyclehook discards connections stamped with a draining node, rate limited to one cull per 500ms so the pool migrates gradually instead of ejecting every affected connection at once and stalling acquires behind a burst of re-establishment. A rate-limited connection is handed out and stays usable until a later acquire culls it (or the server closes it, which the regular recycle check detects).persist_consensus_connection_pool_drain_cull_stagger, default 500ms), mirroring the existing..._ttl_staggerknob so both stagger intervals are tunable without a deploy.persist_consensus_connection_pool_drain_aware_recycling, default off in production, CI default on for CockroachDB-backed metadata stores and off otherwise.Reading
crdb_internal.gossip_livenessis not implied by ordinary database privileges. Before turning this flag on for a deployment, the consensus role needs:On CockroachDB versions before v23.2 the system privilege is not honored for this table and membership in
adminis required instead (verified: on v23.1.4 the grant succeeds but the read still fails with "only users with the admin role are allowed to read crdb_internal.gossip_liveness"; on v25.4.10 the grant works). Stamping connections viacrdb_internal.node_id()needs no special grant.No deployment can enable this flag today without a privilege change first. Environments connect as an unprivileged per-environment role that owns only its own database, so the liveness poll will fail and the feature will disable itself. Granting the privilege is a deliberate decision with real tradeoffs (it is cluster-wide, and on a shared metadata cluster it exposes cross-tenant metadata such as other databases' names and cluster topology), so the flag should stay off until that is worked through. A missing grant degrades rather than breaks.
Known limitation: poll amplification
Every pool polls independently. One production region currently runs ~500 processes with consensus pools, so enabling this fleet-wide at the default 5s interval would add ~100 queries/sec of permanent load to the very metadata cluster the feature is meant to protect, in order to detect drains that happen a handful of times a year. Before this is enabled broadly, the draining-node set is probably better discovered once per cluster by a privileged component and distributed to environments, rather than polled per pool. Filing that as follow-up rather than blocking this PR, which is still useful for single-pool and self-hosted cases.
Graceful degradation on non-CockroachDB backends
This pool is shared with vanilla-Postgres deployments, so enabling the feature where it cannot work must be harmless rather than pathological. Each pool holds a
drain_recycling_supportedflag that starts true and latches false the first time either query fails with a permanent SQLSTATE —3F000/42P01/42883(nocrdb_internal) or42501(role lacks the grant above). After that the pool stops stamping and stops polling, so a misconfigured flag costs one failed query for the life of the pool instead of one per connection plus one every 5s.The two queries do not necessarily fail together, which is why the flag is a latch rather than a "detected backend" value: a read-only role on a real CockroachDB can call
crdb_internal.node_id()but cannot readgossip_liveness, so stamping succeeds and only the poll fails. Verified against all four configurations (vanilla Postgres 16, CockroachDB as admin, CockroachDB as a role without cluster-metadata access, and no backend configured).Design notes
server.shutdown.connections.timeoutat its 0s default there is little time between the gossip flag flipping and the hard close. Pairs with the operational recommendation to raise that timeout.Tests
drain_aware_recycling_culls_stamped_connectionsinmz-postgres-client: verifies node stamping, that healthy-node connections are reused, and that marking a connection's node as draining causes it to be discarded and replaced on the next acquire. Deterministic under the concurrent watchdog (single-connection pool, inequality assertions). Opt-in viaMZ_PERSIST_EXTERNAL_STORAGE_TEST_POSTGRES_URL.pool-exhaustionworkflow intest/crdb-restarts(from persist: pre-warm the consensus connection pool #38053): with this flag on,connections_createdshould rise during each drain window rather than after node restart.Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way), then it is tagged with aT-protolabel.🤖 Generated with Claude Code