Skip to content

Add a thread-local cache disable pool test (depends on database-connector#26)#535

Closed
marknefedov wants to merge 1 commit into
duckdb:mainfrom
marknefedov:fix/pool-tls-cache-disable-leak
Closed

Add a thread-local cache disable pool test (depends on database-connector#26)#535
marknefedov wants to merge 1 commit into
duckdb:mainfrom
marknefedov:fix/pool-tls-cache-disable-leak

Conversation

@marknefedov

Copy link
Copy Markdown

Blocked on duckdb/database-connector#26 — CI will be red until that merges and the submodule pointer is bumped.

What this is

The SQL-level regression test for the thread-local connection cache leak. The fix itself is in the database-connector submodule (duckdb/database-connector#26); only the test belongs in this repo.

The bug

TryAcquireFromThreadLocal stops consulting the cache as soon as tl_cache_enabled goes false, but a connection already parked in that thread's cache is left there. It still counts towards total_connections while being unreachable, so it permanently occupies a pool slot:

Connection pool timeout: all 1 connections in use, waited 100ms

Reachable from this repo through postgres_configure_pool(enable_thread_local_cache := false) on a pool that had the cache enabled and in use.

The test

test/sql/storage/attach_connection_pool_tl_cache_disable.test walks enable → park → shrink → disable → query.

Two things worth flagging for review:

  • It sets threads=1. The connection has to be parked and re-acquired on the same thread for the drain to fire, since the fix only recovers the calling thread's slot.
  • It runs a control query first with the cache off and max_connections=1. If a single connection were not enough for this query, the later assertion could not distinguish a stranded connection from a query that simply needs two — the control makes that failure mode explicit rather than silent.

The pool also has a test/test_pool.cpp unit test in the connector PR, which is the more direct regression. Note that database-connector/test is not wired into this repo's CI, so that one only runs in the connector repo.

To land

  1. Merge Return the parked connection when the thread-local cache is disabled database-connector#26
  2. Bump the database-connector submodule pointer on this branch
  3. CI goes green, undraft

TryAcquireFromThreadLocal stopped consulting the cache as soon as
tl_cache_enabled went false, but a connection already parked in this
thread's cache was left there. It keeps counting towards
total_connections, so it occupies a pool slot that nothing can hand out
again:

  Connection pool timeout: all 1 connections in use, waited 100ms

Hand the parked connection back to the global pool instead. Clear()
already routes through ReturnFromThreadLocalCache, which re-checks
expiry and the max_connections bound, so this reuses the existing return
path.

This only recovers the slot of a thread that acquires again after the
cache was disabled. Dormant worker threads still hold their connection
until their ThreadLocalConnectionCache is destroyed - draining those
would need a generation counter or a registry of live caches.
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