From 60709b8319fa636b0e24a520fd004785566412a5 Mon Sep 17 00:00:00 2001 From: Trenton H <797416+stumpylog@users.noreply.github.com> Date: Wed, 9 Sep 2026 16:47:07 -0700 Subject: [PATCH] Performance: cut redundant per-document lookups in bulk `modify_custom_fields` (#13807) * Perf: batch the repeated lookups in modify_custom_fields `modify_custom_fields()` re-resolved the same objects inside its per-document loop: `custom_fields.get(id=field_id)` re-ran a CustomField query for every document, and doc link fields called `Document.objects.get(id=doc_id)` a second time for a document that was already known. Resolve both up front with `in_bulk()` and hand the resolved objects to `update_or_create()` rather than bare ids. Passing the objects also populates the FK cache on the newly created instance, so auditlog's post_save receiver touching `.document`/`.field` no longer costs a reload per row. The document map defers `content`, the one field here that is both large and unused. The symmetrical-link removal pass and `remove_doclink()` get `select_related()` for the same auditlog reason. Measured over 50 documents, sqlite, audit log enabled: before after add 3 string fields 1502 1054 update 1 string field 451 403 add doc link 851 653 remove doc link 604 354 `update_or_create()` is kept as-is. Dropping it for a hand-rolled get-or-construct loop removes a further ~4 statements per row, but those are the SAVEPOINT/RELEASE pairs of its `transaction.atomic()`, and the `select_for_update()` and IntegrityError fallback that go with them. The (document, field) unique constraint depends on that when two bulk edits overlap, and the wall clock did not move to pay for it (331 ms vs 310 ms for the string case above). Also normalises the field ids to int once at the top so the old dict API, whose keys may arrive as strings, indexes the resolved map correctly. The `if custom_field:` branch it replaces was dead: `.get()` raises DoesNotExist, it never returns None. Co-Authored-By: Claude Opus 5 * Fixes the comment --------- Co-authored-by: Claude Opus 5 --- src/documents/bulk_edit.py | 75 ++++++++++++++++++++------------------ 1 file changed, 40 insertions(+), 35 deletions(-) diff --git a/src/documents/bulk_edit.py b/src/documents/bulk_edit.py index 79a79e6cf..363193d48 100644 --- a/src/documents/bulk_edit.py +++ b/src/documents/bulk_edit.py @@ -299,53 +299,55 @@ def modify_custom_fields( ) -> Literal["OK"]: qs = Document.objects.filter(id__in=doc_ids).only("pk") affected_docs = list(qs.values_list("pk", flat=True)) - # Ensure add_custom_fields is a list of tuples, supports old API + # Ensure add_custom_fields is a list of (int, value) tuples, supports old API add_custom_fields = ( - add_custom_fields.items() + [(int(field), value) for field, value in add_custom_fields.items()] if isinstance(add_custom_fields, dict) - else [(field, None) for field in add_custom_fields] + else [(int(field), None) for field in add_custom_fields] ) - custom_fields = CustomField.objects.filter( - id__in=[int(field) for field, _ in add_custom_fields], - ).distinct() + # Resolved once, instead of re-querying the same field for every document + custom_fields_by_id: dict[int, CustomField] = CustomField.objects.in_bulk( + [field_id for field_id, _ in add_custom_fields], + ) + # Passed to update_or_create() below rather than a bare id, so the FK is + # cached on the created instance and auditlog's post_save receiver does + # not reload it per row. Only needed for additions. content is deferred: + # the one field here that is both large and unused. + docs_by_id: dict[int, Document] = ( + Document.objects.defer("content").in_bulk(affected_docs) + if add_custom_fields + else {} + ) for field_id, value in add_custom_fields: + custom_field = custom_fields_by_id[field_id] + value_field = CustomFieldInstance.TYPE_TO_DATA_STORE_NAME_MAP[ + custom_field.data_type + ] + is_doclink = custom_field.data_type == CustomField.FieldDataType.DOCUMENTLINK for doc_id in affected_docs: - defaults = {} - custom_field = custom_fields.get(id=field_id) - if custom_field: - value_field = CustomFieldInstance.TYPE_TO_DATA_STORE_NAME_MAP[ - custom_field.data_type - ] - defaults[value_field] = value - if ( - custom_field.data_type == CustomField.FieldDataType.DOCUMENTLINK - and value - and doc_id in value - ): - # Prevent self-linking - continue + if is_doclink and value and doc_id in value: + # Prevent self-linking + continue CustomFieldInstance.objects.update_or_create( - document_id=doc_id, - field_id=field_id, - defaults=defaults, + document=docs_by_id[doc_id], + field=custom_field, + defaults={value_field: value}, ) - if custom_field.data_type == CustomField.FieldDataType.DOCUMENTLINK: - doc = Document.objects.get(id=doc_id) - reflect_doclinks(doc, custom_field, value) + if is_doclink: + reflect_doclinks(docs_by_id[doc_id], custom_field, value) - # For doc link fields that are being removed, remove symmetrical links + # For doc link fields that are being removed, remove symmetrical links. + # select_related avoids a per-instance reload of the document and field. for doclink_being_removed_instance in CustomFieldInstance.objects.filter( document_id__in=affected_docs, field__id__in=remove_custom_fields, field__data_type=CustomField.FieldDataType.DOCUMENTLINK, value_document_ids__isnull=False, - ): + ).select_related("field", "document"): for target_doc_id in doclink_being_removed_instance.value: remove_doclink( - document=Document.objects.get( - id=doclink_being_removed_instance.document.id, - ), + document=doclink_being_removed_instance.document, field=doclink_being_removed_instance.field, target_doc_id=target_doc_id, ) @@ -1178,10 +1180,13 @@ def remove_doclink( """ Removes a 'symmetrical' link to `document` from the target document's existing custom field instance """ - target_doc_field_instance = CustomFieldInstance.objects.filter( - document_id=target_doc_id, - field=field, - ).first() + # select_related: a signal receiver (auditlog) touches .document/.field on + # the save() below, without this that is a per-call reload query + target_doc_field_instance = ( + CustomFieldInstance.objects.filter(document_id=target_doc_id, field=field) + .select_related("document", "field") + .first() + ) if ( target_doc_field_instance is not None and document.id in target_doc_field_instance.value