- Notifications
You must be signed in to change notification settings - Fork927
chore(coderd/database): optimize GetRunningPrebuiltWorkspaces#18588
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
base:main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I was a bit thrown off by the indentation, but I tried my best 😆, left some suggestions and questions.
| ready_agents.job_id = latest_prebuilds.job_id | ||
| ORDER BY | ||
| latest_prebuilds.workspace_id ASC | ||
| ; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nit: s/spaces/tabs/
| latest_prebuilds | ||
| AS ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| latest_prebuilds | |
| AS ( | |
| latest_prebuildsAS ( |
We could reduce indentation a bit here.
| FROM | ||
| workspaces | ||
| LEFT JOIN LATERAL ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| FROM | |
| workspaces | |
| LEFT JOIN LATERAL ( | |
| FROM | |
| workspaces | |
| LEFT JOIN LATERAL ( |
As well as here, SQL is a weird syntax in that it doesn't really have any meaningful nesting.
| provisioner_jobs.id | ||
| = workspace_builds.job_id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| provisioner_jobs.id | |
| =workspace_builds.job_id | |
| provisioner_jobs.id=workspace_builds.job_id |
What's this 😆, (see others as well)
| AS ( | ||
| SELECT | ||
| latest_prebuilds.job_id, | ||
| BOOL_AND(workspace_agents.lifecycle_state = 'ready'::workspace_agent_lifecycle_state)::boolean AS ready |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| BOOL_AND(workspace_agents.lifecycle_state='ready'::workspace_agent_lifecycle_state)::booleanAS ready | |
| workspace_agents.lifecycle_state='ready'::workspace_agent_lifecycle_stateAS ready |
What's this, it actually works?
| latest_prebuilds.created_at | ||
| FROM | ||
| latest_prebuilds | ||
| LEFT JOIN ready_agents ON |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This join will duplicate rows when there are multiple agents since there is no group-by here or in theready_agents CTE, was this considered?
| JOIN workspace_agents ON | ||
| workspace_agents.resource_id = workspace_resources.id | ||
| WHERE | ||
| workspace_agents.deleted = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
For consideration: Should we filter out sub agents? I.e.AND workspace_agents.parent_id IS NULL.
| AND latest_build.transition | ||
| = 'start'::workspace_transition | ||
| AND latest_build.job_status | ||
| = 'succeeded'::provisioner_job_status |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Potential performance improvement, move these intolatest_build subquery.
Also, please consider whether it should be aLEFT JOIN LATERAL or(INNER) JOIN LATERAL. With these filters it's essentially an INNER join currently.
| workspace_builds.workspace_id | ||
| = workspaces.id | ||
| ORDER BY | ||
| workspace_builds.workspace_id, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Suggestion: Could remove this redundant ordering due to WHERE filter (keep build_number though).
Uh oh!
There was an error while loading.Please reload this page.
Fixescoder/internal#715
Before: ~44mshttps://explain.dalibo.com/plan/76cbe21d1a4c9329#plan
After: ~3.7mshttps://explain.dalibo.com/plan/f61beg93f9306fc0#plan