Skip to content

persist: proactively recycle connections on draining CRDB nodes [POC] - #38097

Draft
jubrad wants to merge 2 commits into
MaterializeInc:mainfrom
jubrad:crdb-pool-drain-recycle
Draft

persist: proactively recycle connections on draining CRDB nodes [POC]#38097
jubrad wants to merge 2 commits into
MaterializeInc:mainfrom
jubrad:crdb-pool-drain-recycle

Conversation

@jubrad

@jubrad jubrad commented Aug 6, 2026

Copy link
Copy Markdown
Member

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 (see pkg/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 draining flag — and recycles connections off a draining node during the drain grace window instead of after the close.

Changes

  • Each pooled connection is stamped with crdb_internal.node_id() at creation (one extra round trip per create, gated).
  • A background watchdog (spawned lazily on first acquire, aborted on client drop) polls crdb_internal.gossip_liveness every 5s for current cluster members that are draining or decommissioning (draining OR membership != 'active', joined against gossip_nodes to exclude liveness tombstones of long-decommissioned nodes, which linger forever with draining still set). Liveness expiration is 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.
  • The pre_recycle hook 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).
  • The cull rate is itself a dyncfg (persist_consensus_connection_pool_drain_cull_stagger, default 500ms), mirroring the existing ..._ttl_stagger knob so both stagger intervals are tunable without a deploy.
  • Gated behind new dyncfg persist_consensus_connection_pool_drain_aware_recycling, default off in production, CI default on for CockroachDB-backed metadata stores and off otherwise.

⚠️ Required grant before enabling

Reading crdb_internal.gossip_liveness is not implied by ordinary database privileges. Before turning this flag on for a deployment, the consensus role needs:

GRANT SYSTEM VIEWCLUSTERMETADATA TO <role>;   -- CockroachDB v23.2+

On CockroachDB versions before v23.2 the system privilege is not honored for this table and membership in admin is 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 via crdb_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_supported flag that starts true and latches false the first time either query fails with a permanent SQLSTATE — 3F000 / 42P01 / 42883 (no crdb_internal) or 42501 (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 read gossip_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

  • Polling gossip beats probing for the drain error string: probes through a load balancer can't target a node (and health-checked LBs remove draining nodes from rotation, so probes miss them), the refused startup doesn't identify which node is draining, and pgcode is stable while error text isn't.
  • Node IDs are never reused (monotonic allocator; in-place restarts keep their ID), so a tombstone or stale set entry can never match a live connection's stamp.
  • Effectiveness depends on the drain actually having a grace window: with server.shutdown.connections.timeout at 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_connections in mz-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 via MZ_PERSIST_EXTERNAL_STORAGE_TEST_POSTGRES_URL.
  • End-to-end validation against a real rolling drain can use the pool-exhaustion workflow in test/crdb-restarts (from persist: pre-warm the consensus connection pool #38053): with this flag on, connections_created should rise during each drain window rather than after node restart.

Checklist

  • This PR has adequate test coverage / QA involvement has been duly considered. (trigger-ci for additional test/nightly runs)
  • This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
  • If this PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way), then it is tagged with a T-proto label.
  • If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
  • If this PR includes major user-facing behavior changes, I have pinged the relevant PM to schedule a changelog post.

🤖 Generated with Claude Code

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 jubrad changed the title persist: proactively recycle connections on draining CRDB nodes persist: proactively recycle connections on draining CRDB nodes [DNM] - just for fun Aug 7, 2026
@jubrad jubrad changed the title persist: proactively recycle connections on draining CRDB nodes [DNM] - just for fun persist: proactively recycle connections on draining CRDB nodes [POC] Aug 7, 2026
@jubrad
jubrad force-pushed the crdb-pool-drain-recycle branch 5 times, most recently from 1aeedad to c74f117 Compare August 7, 2026 17:59
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
jubrad force-pushed the crdb-pool-drain-recycle branch from c74f117 to 4b640f7 Compare August 7, 2026 18:24
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.

1 participant