serve: a pool of sessions behind an executor interface - #322
Merged
Merged
Conversation
5 tasks
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.
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 serveanswered 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:Throughput stops climbing at four clients and latency grows in proportion. The same request reported
elapsed_msof 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
Executorinterface:internal/serve/executor.goholdsExecutor,SessionandStatementplus the pool;internal/serve/duckdb.gois 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.Session.Runreturns anarray.RecordReaderand 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.SET TimeZoneis session-scoped, so a session that missed it evaluatesdate_truncand naive casts in the host's zone — wrong buckets from a correct config, on some requests and not others.commandsrun once, on a connection of their own:ATTACHis database-wide and attaching the same alias twice errors.queued_msjoinselapsed_ms, which is now the query alone./healthzanswersbusyrather thanunavailablewhen no session is free, and answersHEAD, which monitors send andgetOnlyrefused with 405.HEADstays refused elsewhere: aHEADof a dataset would run the query, borrow a session and discard the rows.GET /metricson the existing listener whenserve.metrics.enabledis set, with six instruments includingsqlflow_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):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_PostgresBackendsStayBoundedmeasured 4, with eight sessions scanning 400k rows at once againstpg_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, neitheradbc.Statementnor the driver manager exposes one. ButExecuteQuerydocuments that releasing the returned reader without consuming it is equivalent toAdbcStatementCancel. So the reader is the cancel; there is simply no method another goroutine can call.readRowstherefore 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 atoptions='-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 failuresgo test -run '^TestIntegration' ./internal/serve/ ./internal/rollup/: bothokuv run --locked pytest tests/tooling -q: 205 passedgo build ./...,go vet ./...,gofmt -lall cleanuv 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 themissinga 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:
PoolRunsQueriesConcurrentlyandAPoolOfOneSerializesare a matched pair: four 200 ms queries finish in 0.20 s, three 100 ms queries take 0.30 s.EverySessionIsUTCruns underTZ=America/New_Yorkand holds every session at once. Removing the pin fails it, reportingAmerica/New_York.CloseWaitsForBorrowedSessionsfails if Close stops waiting; a session closed mid-query takes the process down.ReadRowsStopsWhenTheRequestIsGonefails if the context check is removed.QueuedMsSeparatesWaitFromWorkfails if the two are merged again.Notes for the reviewer
internal/serve/query.gois gone:statement,prepareandrecordmoved toduckdb.goasduckdbStatement, and the oldexecutorbecame the pool.TestCliServe_HealthzAnswersHeadasserts the status only.httptest.NewRecorderhands back the handler's body verbatim, where a realhttp.Serversuppresses it for HEAD, so an empty-body assertion would be testing the recorder.