- Notifications
You must be signed in to change notification settings - Fork1k
chore: improve performance of 'GetLatestWorkspaceBuildsByWorkspaceIDs'#19452
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
chore: improve performance of 'GetLatestWorkspaceBuildsByWorkspaceIDs'#19452
Uh oh!
There was an error while loading.Please reload this page.
Conversation
The query change makes sense. I noticed that we're not enforcing a value to the limit parameter and we're also not logging (and I can't find tracing spans that have this info either) whether a limit value was passed, how many workspace IDs were passed, which pagination offset was passed, etc. All of which would help us identify whether the P99 is related to any of those. |
We probably should have a sensible limit |
It's hard to know where to put the upper limit tbh without some real-world data. Let's revisit this idea when we do. |
ef0d74f
intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Closescoder/internal#716
This prevents a scan over the entire
workspace_build
table by removing ajoin
. This is still imperfect as we are still scanning over the number of builds for the workspaces in the arguments. Ideally we would have some index or something precomputed. Then we could skip scanning over the builds for the correct workspaces that are not the latest.So this could be better, but this is a quick & cheap order of magnitude win.
Before: (69k rows scanned)
https://explain.dalibo.com/plan/d91d553ge694a18c
After: (2,500 rows scanned)
https://explain.dalibo.com/plan/6ec4eehde8c381a7#plan
Query used hits 2 workspaces with >1k builds.
Other notes
I tried to mess around with some indexes like: (plan)
This did not seem to help over theexisting index without the
DESC
. (plan)Both indexs still scan over all the rows (105 in the plans).
Some latest evidence on dev