Skip to content

serve: a pool of sessions behind an executor interface - #322

Merged
turbolytics merged 12 commits into
mainfrom
feat/serve-pool
Sep 17, 2026
Merged

turbolytics merged 12 commits into
mainfrom
feat/serve-pool

Conversation

@turbolytics

Copy link
Copy Markdown
Owner

Implements #320, and contains that PR's spec and plan commits, so #320 can be closed in favour of this one.

What this changes

sqlflow serve answered one request at a time: every request waited on one mutex around one DuckDB connection. Measured against the Bluesky demo on Render on 2026-09-16, after rollup tables had already cut the query itself to 13 ms:

Clients Throughput p50 p95
1 5.8/s 164 ms 251 ms
4 12.2/s 303 ms 418 ms
16 13.4/s 1,191 ms 1,318 ms
32 13.2/s 2,328 ms 2,682 ms

Throughput stops climbing at four clients and latency grows in proportion. The same request reported elapsed_ms of 13 idle and 945–1,219 under sixteen clients: the query was unchanged, and the rest was queueing counted as work.

Requests now run on a pool of sessions behind an Executor interface:

  • The seam is "run this dataset statement", not "give me a connection". internal/serve/executor.go holds Executor, Session and Statement plus the pool; internal/serve/duckdb.go is the only file in the package that imports ADBC. The interface exists because the engine underneath may change — the Postgres sink had to leave DuckDB's postgres extension for pgx in A keyed Postgres sink on pgx, and the leak loops that found the demo's growth #290, and that move was expensive because the driver was welded into the write path.
  • Arrow stays the interchange. Session.Run returns an array.RecordReader and serve keeps one encoder, so the four documented rendering rules (UTC timestamps, naive timestamps, exact decimals, NaN as a string) hold for any backend rather than being re-implemented per backend.
  • serve.pool.size, four by default, 0 meaning the default and 64 the ceiling.
  • Every session is pinned to UTC by serve, not by the config. SET TimeZone is session-scoped, so a session that missed it evaluates date_trunc and naive casts in the host's zone — wrong buckets from a correct config, on some requests and not others.
  • commands run once, on a connection of their own: ATTACH is database-wide and attaching the same alias twice errors.
  • queued_ms joins elapsed_ms, which is now the query alone.
  • /healthz answers busy rather than unavailable when no session is free, and answers HEAD, which monitors send and getOnly refused with 405. HEAD stays refused elsewhere: a HEAD of a dataset would run the query, borrow a session and discard the rows.
  • GET /metrics on the existing listener when serve.metrics.enabled is set, with six instruments including sqlflow_serve_session_wait_seconds, the one that sizes the pool.

Two measurements that changed the design

Memory, in the release image on Linux (memory_limit='128MB', the demo's widest permitted fold):

Sessions Idle Peak
1 42 MiB 53 MiB
4 41 MiB 72 MiB
8 41 MiB 101 MiB

Four sessions fit a 256 MB box with a 2× margin, so the default is 4. Idle sessions cost nothing measurable; the cost is per concurrent query. Linux came in about 27% cheaper than macOS, so the spec's original table overstated it.

Postgres connections. The spec assumed the worst case — pool.size × pg_connection_limit, up to 32. TestIntegrationServePool_PostgresBackendsStayBounded measured 4, with eight sessions scanning 400k rows at once against pg_connection_limit = 4. The attachment's connections are shared across sessions, not opened per session, so a pool costs sessions and memory but not database connections. The README, the spec and the horizontal-scaling analysis all said the pessimistic thing and are corrected; the test asserts the measured bound, so a DuckDB release that changed it fails rather than quietly making the docs stale.

Cancellation, which the spec had wrong

ADBC's Go API has no Cancel — checked against v1.6.0, neither adbc.Statement nor the driver manager exposes one. But ExecuteQuery documents that releasing the returned reader without consuming it is equivalent to AdbcStatementCancel. So the reader is the cancel; there is simply no method another goroutine can call.

readRows therefore takes the request's context and stops at the first batch boundary after it ends, and the reader is released, which cancels. A single operator that runs long before yielding a batch still runs to the end holding its session — the README now points at options='-c statement_timeout=30000' in the attach string for that, which Postgres enforces rather than sqlflow hoping.

Verification

  • go test -short -race ./...: 25 packages, 0 failures
  • go test -run '^TestIntegration' ./internal/serve/ ./internal/rollup/: both ok
  • uv run --locked pytest tests/tooling -q: 205 passed
  • go build ./..., go vet ./..., gofmt -l all clean
  • uv run --locked pytest tests/release -q: not run locally. cli.serve's release coverage is unchanged by this PR and comes from CI; the committed status file keeps the value CI measured rather than the missing a local run without the image computes.
  • make soak: not run. Nothing in the consume loop, a handler or a sink changed.

Tests that can fail, rather than pass vacuously:

  • PoolRunsQueriesConcurrently and APoolOfOneSerializes are a matched pair: four 200 ms queries finish in 0.20 s, three 100 ms queries take 0.30 s.
  • EverySessionIsUTC runs under TZ=America/New_York and holds every session at once. Removing the pin fails it, reporting America/New_York.
  • CloseWaitsForBorrowedSessions fails if Close stops waiting; a session closed mid-query takes the process down.
  • ReadRowsStopsWhenTheRequestIsGone fails if the context check is removed.
  • QueuedMsSeparatesWaitFromWork fails if the two are merged again.

Notes for the reviewer

  • internal/serve/query.go is gone: statement, prepare and record moved to duckdb.go as duckdbStatement, and the old executor became the pool.
  • The leak test's warmup went from 50 to 500 requests. A pool reaches steady state later than one connection did — measured over four consecutive runs the process grew 10 MiB, then 3, then 1, then 0, a plateau. Fifty requests sampled the middle of that climb and read it as growth.
  • TestCliServe_HealthzAnswersHead asserts the status only. httptest.NewRecorder hands back the handler's body verbatim, where a real http.Server suppresses it for HEAD, so an empty-body assertion would be testing the recorder.
  • A session returns to the pool when its query finishes, never when a caller gives up. Handing a session to the next request while a query still runs on it would serialise them behind work nobody wants.
  • Caching is the next spec, and spec: a pool of serve sessions behind an executor interface #320's "Scaling out" section argues it is the prerequisite for running more than one instance rather than a latency optimisation.

@turbolytics
turbolytics merged commit 69ae0af into main Sep 17, 2026
5 checks passed
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