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

Eliminate usage of bulk_create() and bulk_update() methods outside of tests and migrations #10694

Closed
jeremystretch opened this issue Oct 19, 2022 · 3 comments
Assignees
Labels
status: accepted This issue has been accepted for implementation type: housekeeping Changes to the application which do not directly impact the end user
Milestone

Comments

@jeremystretch
Copy link
Member

jeremystretch commented Oct 19, 2022

Proposed Changes

While we've generally avoided the use of bulk_create() and bulk_update() for the bulk creation and manipulation of objects due to the caveats associated with their use (namely not triggering the post_save signal), they are still used in a few areas within NetBox. The goal of this issue is to eliminate any usage of these methods, excluding within tests and migrations where specifically needed to ensure optimal performance.

Justification

As calling bulk_create() or bulk_update() does not generate a post_save signal for each of the objects affected, this escapes change logging and (in NetBox v3.4) search indexing. Although calling save() on each instance individual will impose a performance penalty, it ensures that each instance is subject to the appropriate background workflows. (This was initially called out here by @kkthxbye-code.)

@jeremystretch jeremystretch added status: accepted This issue has been accepted for implementation type: housekeeping Changes to the application which do not directly impact the end user labels Oct 19, 2022
@arthanson arthanson changed the title Eliminate usage of bulk_create() and bulk_create() methods outside of tests and migrations Eliminate usage of bulk_create() and bulk_update() methods outside of tests and migrations Oct 27, 2022
@arthanson
Copy link
Collaborator

arthanson commented Oct 27, 2022

bulk_create used in:

  • netbox/dcim/models/devices.py
  • netbox/search/backends.py

bulk_update used in:

  • netbox/dcim/models/devices.py
  • netbox/extras/models/customfields.py
  • netbox/ipam/signals.py
  • netbox/ipam/utils.py

@jeremystretch
Copy link
Member Author

The goal of this issue is to eliminate any usage of these methods

Rather than eliminating their usage, which would incur significant performance penalties when performing certain operations, we could instead manually trigger the post_save signal for each object after the bulk operation is run. I'm a little hesitant to commit to this approach as it alters Django's default behavior, however it might be reasonable for our purposes. I'm going to put in a PR to test.

@jeremystretch jeremystretch added this to the v3.4 milestone Nov 14, 2022
jeremystretch added a commit that referenced this issue Nov 14, 2022
…s in bulk (#10900)

* Emit post_save signal when creating/updating device components in bulk

* Fix post_save for bulk_update()
@jeremystretch
Copy link
Member Author

The hybrid approach I mentioned above seems to save a substantial amount of time compared to completely removing the bulk calls (despite still being far slower than the current implementation, which does not support change logging). I've merged PR #10900 using this approach and will test during the v3.4 beta release. Should any serious issues with this approach surface, we'll revert to the approach in PR #10780.

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

2 participants