[DB] Remove extra get user query from override_request_env_and_config on postgres #7676
Conversation
|
/quicktest-core |
|
I do think SQLite supports RETURNING, https://sqlite.org/lang_returning.html |
You're right, thanks for pointing it out! I just assumed there isn't since we weren't using it on the sqlite code path. |
Note: The RETURNING syntax has been supported by SQLite since version 3.35.0 (2021), but SQLAlchemy only added support for it since 2.0 (2023). In any case, I've added a check for both to fallback to doing an additional query in case the sqlite/sqlalchemy version being used does not support it yet. References: |
|
I verified that the sqlite and sqlalchemy version that our docker image has by default does support RETURNING: % docker run berkeleyskypilot/skypilot-nightly:latest pip show sqlalchemy | grep Version
Version: 2.0.44
% docker run berkeleyskypilot/skypilot-nightly:latest python -c 'import sqlite3; print(sqlite3.sqlite_version)'
3.46.1 |
|
/smoke-test 🟢 |
SeungjinYang
left a comment
There was a problem hiding this comment.
LGTM! Couple of minor syntactic nits
|
/smoke-test -k minimal --kubernetes |
Instead of making a separate query to
get_userafteradd_or_update_user, we should return the user object directly from the upsert operation by using theRETURNINGclause (when possible, see discussion below regarding sqlite-specific handling).This reduces one RTT to the DB for every request execution.
Benchmarked with
multitime -n 50 sky status --no-show-managed-jobs --no-show-services --no-show-pools:Before:
After:
Mean and median decreases by around 1 RTT, also lower std dev and max, which is expected.
Tested (run the relevant ones):
bash format.sh/smoke-test(CI) orpytest tests/test_smoke.py(local)/smoke-test -k test_name(CI) orpytest tests/test_smoke.py::test_name(local)/quicktest-core(CI) orpytest tests/smoke_tests/test_backward_compat.py(local)