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

Evaluate replacing django-mptt with CTE #6587

Closed
jeremystretch opened this issue Jun 10, 2021 · 9 comments
Closed

Evaluate replacing django-mptt with CTE #6587

jeremystretch opened this issue Jun 10, 2021 · 9 comments
Labels
status: accepted This issue has been accepted for implementation type: housekeeping Changes to the application which do not directly impact the end user

Comments

@jeremystretch
Copy link
Member

Proposed Changes

With the upcoming v3.0 release and the recent loss of maintainership for django-mptt, this is a good time to evaluate replacing our usage of MPTT for hierarchical models with an alternate solution, namely PostgreSQL common table expressions (CTE).

When working on #6087 I came across django-tree-queries, which seems like a neat, lightweight implementation of CTE for Django. We should look into using it, or at replicating its approach within NetBox.

Justification

MPTT has serves us pretty well, however it comes with some limitations, notably the need to consistently rebuild the tree with every change. This imposes some additional overhead around things like bulk updates. MPTT also requires a set of database fields to maintain the tree: tree_id, level, lft (left), and rght (right).

A CTE-based approach to conveying recursively hierarchies does not impose this requirement, as the hierarchy is built at query time rather than at write time. It remains to be seen, however, what sort of performance penalty this imposes.

@jeremystretch jeremystretch added type: housekeeping Changes to the application which do not directly impact the end user status: under review Further discussion is needed to determine this issue's scope and/or implementation labels Jun 10, 2021
@jeremystretch jeremystretch added this to the v3.0 milestone Jun 11, 2021
@jeremystretch
Copy link
Member Author

Tagging this for v3.0 because I want to at least make the evaluation prior to the beta release.

@jeremystretch jeremystretch self-assigned this Jun 14, 2021
@jeremystretch
Copy link
Member Author

Initial testing using django-tree-queries has been promising with regard to performance, however we'll need to devise a suitable replacement for django-mptt's add_related_count() manager method for counting related objects. Replicating the approach directly won't work, because Django's Subquery won't work with the CTE annotations (FieldError: Cannot resolve keyword 'tree_path' into field.).

I've opened feincms/django-tree-queries#21 to raise this issue and see if anyone can offer some guidance.

@jeremystretch jeremystretch removed this from the v3.0 milestone Jun 30, 2021
@jeremystretch jeremystretch removed their assignment Aug 12, 2021
@github-actions
Copy link
Contributor

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. NetBox is governed by a small group of core maintainers which means not all opened issues may receive direct feedback. Please see our contributing guide.

@github-actions github-actions bot added the pending closure Requires immediate attention to avoid being closed for inactivity label Oct 11, 2021
@jeremystretch
Copy link
Member Author

I'm going to tag this to revisit for v3.2.

@jeremystretch jeremystretch added status: accepted This issue has been accepted for implementation and removed status: under review Further discussion is needed to determine this issue's scope and/or implementation pending closure Requires immediate attention to avoid being closed for inactivity labels Oct 11, 2021
@jeremystretch jeremystretch added this to the v3.2 milestone Oct 11, 2021
@jeremystretch jeremystretch removed this from the v3.2 milestone Dec 8, 2021
@jeremystretch jeremystretch added needs milestone Awaiting prioritization for inclusion with a future NetBox release and removed status: accepted This issue has been accepted for implementation labels Dec 8, 2021
@pfcodes
Copy link

pfcodes commented Nov 30, 2022

@jeremystretch came across this post via a google search. were there any pitfalls to switching from mptt to cte, if you went through with doing so?

@arthanson
Copy link
Collaborator

Author of django-tree-queries lists his reasons here: https://406.ch/writing/django-tree-queries/. He notes potential issues he doesn't like with PostgreSQL ltree, however this is probably still a viable alternative. See https://github.com/mariocesar/django-ltree and https://github.com/peopledoc/django-ltree-demo (the ltree-demo could just be pulled into the app and updated, i.e. not use any library). Potentially easier to implement the equivalent of add_related_count()...

@arthanson
Copy link
Collaborator

Also see #11421

@arthanson arthanson added this to the v3.5 milestone Jan 6, 2023
@arthanson arthanson self-assigned this Jan 6, 2023
@ryanmerolle ryanmerolle removed the needs milestone Awaiting prioritization for inclusion with a future NetBox release label Jan 12, 2023
@jeremystretch jeremystretch added the status: accepted This issue has been accepted for implementation label Feb 7, 2023
@jeremystretch jeremystretch modified the milestones: v3.5, v3.6 Feb 7, 2023
@abhi1693
Copy link
Member

There is also a repo which may be used to build our own logic. I just came across this and haven't had a chance to look in depth.

https://github.com/dimagi/django-cte

@jeremystretch
Copy link
Member Author

Wrapping this into #12552.

@jeremystretch jeremystretch closed this as not planned Won't fix, can't repro, duplicate, stale May 10, 2023
@jeremystretch jeremystretch removed this from the v3.6 milestone May 12, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: accepted This issue has been accepted for implementation type: housekeeping Changes to the application which do not directly impact the end user
Projects
None yet
Development

No branches or pull requests

5 participants