Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Duplicate queries and slow performance with locations #4573

Closed
Kircheneer opened this issue Oct 2, 2023 · 6 comments · Fixed by #4594
Closed

Duplicate queries and slow performance with locations #4573

Kircheneer opened this issue Oct 2, 2023 · 6 comments · Fixed by #4594
Assignees
Labels
type: bug Something isn't working as expected

Comments

@Kircheneer
Copy link
Contributor

Kircheneer commented Oct 2, 2023

Environment

  • Nautobot version: 1.6.2
  • Python version: 3.8.18
  • Database platform, version: postgres:13-alpine
  • Middleware(s): -

Steps to Reproduce

  1. Create a number of locations with one level of nesting (e.g. a floor and a room location type)
  2. Browse to /api/dcim/locations
  3. Observe the SQL query log, which indicates a 50-times duplication of the following query:
WITH RECURSIVE __tree ( "tree_depth", "tree_path", "tree_ordering", "tree_pk" ) AS ( SELECT 0 AS tree_depth, array[T.id] AS tree_path, array["name"]::text[] AS tree_ordering, T."id" FROM dcim_locationtype T WHERE T."parent_id" IS NULL UNION ALL SELECT __tree.tree_depth + 1 AS tree_depth, __tree.tree_path || T.id, __tree.tree_ordering || "name"::text, T."id" FROM dcim_locationtype T JOIN __tree ON T."parent_id" = __tree.tree_pk ) SELECT (__tree.tree_depth) AS "tree_depth", (__tree.tree_path) AS "tree_path", (__tree.tree_ordering) AS "tree_ordering", "dcim_locationtype"."id", "dcim_locationtype"."created", "dcim_locationtype"."last_updated", "dcim_locationtype"."_custom_field_data", "dcim_locationtype"."parent_id", "dcim_locationtype"."name", "dcim_locationtype"."slug", "dcim_locationtype"."description", "dcim_locationtype"."nestable" FROM "dcim_locationtype" , "__tree" WHERE ("dcim_locationtype"."id" = '46c12bce-7f05-493f-983b-3b027acc07f2'::uuid AND (__tree.tree_pk = dcim_locationtype.id)) ORDER BY ("__tree".tree_ordering) ASC LIMIT 21

Expected Behavior

The API endpoint for locations or anything related to it is performant and doesn't perform an excessive amount of duplicate queries.

Observed Behavior

In a specific deployment with >100k locations, this is causing all endpoints that need to access Location.display (such as both the normal and the nested location API serializer) to exhibit very bad performance, because each individual query takes >1s, quickly summing to a minute for a normal pagination size.

Notes

While unfortunately I am unable to reproduce the terrible performance outside of the specific environment, the duplication of queries is easily reproduced. If I then replace the display implementations of both LocationType and Location, there are no duplicated queries to the location tree left.

@Kircheneer Kircheneer added type: bug Something isn't working as expected triage This issue is new and has not been reviewed. labels Oct 2, 2023
@Kircheneer
Copy link
Contributor Author

I don't know if this makes sense, but perhaps we could cache the queries to the tree for the duration of a single HTTP request? I searched for performance guidelines on the related project but didn't find anything very useful yet.

@HanlinMiao
Copy link
Contributor

Yeah, this is an unfortunate side effect of how we compute the Location class' display property today. I wonder instead of the @Property decorator, if @cached_property might be better for this use case?

@HanlinMiao HanlinMiao removed the triage This issue is new and has not been reviewed. label Oct 3, 2023
@Kircheneer
Copy link
Contributor Author

I see this, but should a single one of those .ancestors() queries take more then an entire second? I know that with >100k locations we have a fair amount of locations, but the individual trees each should be no larger then ~100 nodes which should be fairly swift of a query, shouldn't it?

@glennmatthews
Copy link
Contributor

In 2.0, we added some basic caching of TreeModel.display fields as a part of #3702. Might be possible to pull part of that logic back into the LTM branch?

@Kircheneer
Copy link
Contributor Author

I have submitted a backport PR for the relevants bits of #3702 here: #4594

@Kircheneer
Copy link
Contributor Author

Closed with 1.6.4, confirmed working.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type: bug Something isn't working as expected
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants