fix(sqllab): release DB connection before fetching query results from backend - #43371
fix(sqllab): release DB connection before fetching query results from backend#43371justinpark wants to merge 3 commits into
Conversation
… backend Close the SQLAlchemy session before the S3 fetch and the CPU-bound decompress/deserialize/expand work in SqlExecutionResultsCommand. That work doesn't need the DB, but holding a connection for its duration (which can outlast this endpoint's client-side timeout on large results) was exhausting the small per-worker connection pool when several large-result downloads land concurrently on the same gunicorn worker. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
dosubot
Bot
added
change:backend
Code Review Agent Run #86b21aActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Add two regression tests for SqlExecutionResultsCommand.validate(): - the DB session is closed before the results-backend fetch, so a slow S3 fetch does not hold a connection out of the pool - the query's database relationship is warmed (loaded) before that close, so accessing it later (as run() -> _deserialize_results_payload does) works on the now-detached instance without needing a live session Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #43371 +/- ##
==========================================
+ Coverage 66.73% 66.80% +0.07%
==========================================
Files 2876 2876
Lines 164278 164062 -216
Branches 37900 37860 -40
==========================================
- Hits 109636 109609 -27
+ Misses 52479 52280 -199
- Partials 2163 2173 +10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
raise_for_access() reads g.user, so the two new tests need the same test_request_context()/override_user() wrapping test_run_succeeds already uses. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Code Review Agent Run #d58580Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
1 participant
SUMMARY
SqlExecutionResultsCommand(used by the SQL Lab results endpoint) kept the SQLAlchemy session/connection open for the entire duration of the S3/results-backend fetch and the CPU-bound decompress → deserialize → row-expansion work that follows. None of that work touches the database, but holding a connection open for it — which can run well past this endpoint's client-side timeout for large results — was exhausting the small per-worker SQLAlchemy connection pool whenever several large-result downloads landed concurrently on the same gunicorn worker.This change warms the
query.databaserelationship (needed later by_deserialize_results_payloadfordb_engine_spec) while the session is still open, then closes the session before fetching from the results backend, releasing the connection back to the pool for the remainder of the request.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A - backend-only change, no UI impact.
TESTING INSTRUCTIONS
/api/v1/sqllab/results/while other large-result requests are in flight on the same worker.QueuePool limit ... overflowerrors), and that results are still returned correctly.superset/commands/sql_lab/results.pyshould continue to pass.ADDITIONAL INFORMATION