From e5fc23f2f31a4fbac34e04a4de99400ee8a4ff66 Mon Sep 17 00:00:00 2001 From: Jason Novinger Date: Fri, 12 Sep 2025 16:09:54 -0600 Subject: [PATCH 1/3] Fixes #20327: Device queries are now faster when including ConfidContexts Move .distinct() from main queryset to tag subquery to eliminate performance bottleneck when querying devices with config contexts. The .distinct() call on the main device queryset was causing PostgreSQL to sort all devices before pagination, resulting in 15x slower API responses for large installations (10k+ devices, 100+ config contexts). Moving .distinct() to the tag subquery eliminates duplicates at their source (GenericForeignKey tag relationships) while preserving the fix for issues #5314 and #5387 without impacting overall query performance. --- netbox/extras/querysets.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/netbox/extras/querysets.py b/netbox/extras/querysets.py index cb7c1b0aa37..ee2afed4c9f 100644 --- a/netbox/extras/querysets.py +++ b/netbox/extras/querysets.py @@ -93,7 +93,7 @@ def annotate_config_context_data(self): _data=EmptyGroupByJSONBAgg('data', ordering=['weight', 'name']) ).values("_data").order_by() ) - ).distinct() + ) def _get_config_context_filters(self): # Construct the set of Q objects for the specific object types @@ -117,7 +117,7 @@ def _get_config_context_filters(self): ).values_list( 'tag_id', flat=True - ) + ).distinct() ) ) | Q(tags=None), is_active=True, From f51b377f639ad303f26cb19d9559187b03341f80 Mon Sep 17 00:00:00 2001 From: Jason Novinger Date: Fri, 12 Sep 2025 16:16:03 -0600 Subject: [PATCH 2/3] Add performance regression test for config context annotation The test verifies that: - Main device queries do not use expensive DISTINCT operations - Tag subqueries properly use DISTINCT to prevent duplicates from issue #5387 This ensures the optimization from issue #20327 (moving .distinct() from maintaining query to tag subquery) cannot be accidentally reverted while maintaining the correctness guarantees for issues #5314 and #5387. --- netbox/extras/tests/test_models.py | 74 ++++++++++++++++++++++-------- 1 file changed, 54 insertions(+), 20 deletions(-) diff --git a/netbox/extras/tests/test_models.py b/netbox/extras/tests/test_models.py index 905ceff8823..2357fb9c517 100644 --- a/netbox/extras/tests/test_models.py +++ b/netbox/extras/tests/test_models.py @@ -1,8 +1,10 @@ +import re import tempfile from pathlib import Path from django.contrib.contenttypes.models import ContentType from django.core.files.uploadedfile import SimpleUploadedFile +from django.db import connection from django.forms import ValidationError from django.test import tag, TestCase @@ -536,6 +538,34 @@ def test_virtualmachine_site_context(self): vms[1].get_config_context() ) + def test_valid_local_context_data(self): + device = Device.objects.first() + device.local_context_data = None + device.clean() + + device.local_context_data = {"foo": "bar"} + device.clean() + + def test_invalid_local_context_data(self): + device = Device.objects.first() + + device.local_context_data = "" + with self.assertRaises(ValidationError): + device.clean() + + device.local_context_data = 0 + with self.assertRaises(ValidationError): + device.clean() + + device.local_context_data = False + with self.assertRaises(ValidationError): + device.clean() + + device.local_context_data = 'foo' + with self.assertRaises(ValidationError): + device.clean() + + @tag('regression') def test_multiple_tags_return_distinct_objects(self): """ Tagged items use a generic relationship, which results in duplicate rows being returned when queried. @@ -573,6 +603,7 @@ def test_multiple_tags_return_distinct_objects(self): self.assertEqual(ConfigContext.objects.get_for_object(device).count(), 1) self.assertEqual(device.get_config_context(), annotated_queryset[0].get_config_context()) + @tag('regression') def test_multiple_tags_return_distinct_objects_with_separate_config_contexts(self): """ Tagged items use a generic relationship, which results in duplicate rows being returned when queried. @@ -621,32 +652,35 @@ def test_multiple_tags_return_distinct_objects_with_separate_config_contexts(sel self.assertEqual(ConfigContext.objects.get_for_object(device).count(), 2) self.assertEqual(device.get_config_context(), annotated_queryset[0].get_config_context()) - def test_valid_local_context_data(self): - device = Device.objects.first() - device.local_context_data = None - device.clean() - - device.local_context_data = {"foo": "bar"} - device.clean() + @tag('performance', 'regression') + def test_config_context_annotation_query_optimization(self): + """ + Regression test for issue #20327: Ensure config context annotation + doesn't use expensive DISTINCT on main query. - def test_invalid_local_context_data(self): + Verifies that DISTINCT is only used in tag subquery where needed, + not on the main device query which is expensive for large datasets. + """ + # Use existing test device from ConfigContextTest setup device = Device.objects.first() - device.local_context_data = "" - with self.assertRaises(ValidationError): - device.clean() + # connection.queries_log.clear() - device.local_context_data = 0 - with self.assertRaises(ValidationError): - device.clean() + # Execute annotation + list(Device.objects.filter(pk=device.pk).annotate_config_context_data()) - device.local_context_data = False - with self.assertRaises(ValidationError): - device.clean() + device_queries = [q['sql'] for q in connection.queries_log if 'dcim_device' in q['sql']] - device.local_context_data = 'foo' - with self.assertRaises(ValidationError): - device.clean() + for query in device_queries: + # Main device query should NOT use DISTINCT + self.assertNotIn('SELECT DISTINCT "dcim_device"', query) + + # Tag sub-query _should_ use DISTINCT + tag_distinct_pattern = re.compile( + r'SELECT DISTINCT \w+\."tag_id".*FROM "extras_taggeditem" \w+ INNER JOIN "django_content_type"' + ) + distinct_tag_pattern_found = any(tag_distinct_pattern.search(query) for query in device_queries) + self.assertTrue(distinct_tag_pattern_found) class ConfigTemplateTest(TestCase): From 52c9e5429f873b7e3426589951a2e236d00ea1d8 Mon Sep 17 00:00:00 2001 From: Jason Novinger Date: Mon, 15 Sep 2025 11:09:10 -0500 Subject: [PATCH 3/3] Address PR feedback, clean up new regression test The new regression test now avoids casting the query to a string and inspecting the string, which was brittle at best. The new approach asserts directly against `queryset.distinct` for the main query and then finds the subquery that we expect to have distinct set and verifies that is in fact the case. I also realized that the use of `connection.query_log` was problematic, in that it didn't seem to return any queries as expected. This meant that the test was actually not making any assertions since none of the code inside of the for loop over `device_queries` ever ran. --- netbox/extras/tests/test_models.py | 54 ++++++++++++++++++------------ 1 file changed, 32 insertions(+), 22 deletions(-) diff --git a/netbox/extras/tests/test_models.py b/netbox/extras/tests/test_models.py index 2357fb9c517..7b2e58646af 100644 --- a/netbox/extras/tests/test_models.py +++ b/netbox/extras/tests/test_models.py @@ -1,16 +1,14 @@ -import re import tempfile from pathlib import Path from django.contrib.contenttypes.models import ContentType from django.core.files.uploadedfile import SimpleUploadedFile -from django.db import connection from django.forms import ValidationError from django.test import tag, TestCase from core.models import DataSource, ObjectType from dcim.models import Device, DeviceRole, DeviceType, Location, Manufacturer, Platform, Region, Site, SiteGroup -from extras.models import ConfigContext, ConfigContextProfile, ConfigTemplate, ImageAttachment, Tag +from extras.models import ConfigContext, ConfigContextProfile, ConfigTemplate, ImageAttachment, Tag, TaggedItem from tenancy.models import Tenant, TenantGroup from utilities.exceptions import AbortRequest from virtualization.models import Cluster, ClusterGroup, ClusterType, VirtualMachine @@ -661,26 +659,38 @@ def test_config_context_annotation_query_optimization(self): Verifies that DISTINCT is only used in tag subquery where needed, not on the main device query which is expensive for large datasets. """ - # Use existing test device from ConfigContextTest setup device = Device.objects.first() - - # connection.queries_log.clear() - - # Execute annotation - list(Device.objects.filter(pk=device.pk).annotate_config_context_data()) - - device_queries = [q['sql'] for q in connection.queries_log if 'dcim_device' in q['sql']] - - for query in device_queries: - # Main device query should NOT use DISTINCT - self.assertNotIn('SELECT DISTINCT "dcim_device"', query) - - # Tag sub-query _should_ use DISTINCT - tag_distinct_pattern = re.compile( - r'SELECT DISTINCT \w+\."tag_id".*FROM "extras_taggeditem" \w+ INNER JOIN "django_content_type"' - ) - distinct_tag_pattern_found = any(tag_distinct_pattern.search(query) for query in device_queries) - self.assertTrue(distinct_tag_pattern_found) + queryset = Device.objects.filter(pk=device.pk).annotate_config_context_data() + + # Main device query should NOT use DISTINCT + self.assertFalse(queryset.query.distinct) + + # Check that tag subqueries DO use DISTINCT by inspecting the annotation + config_annotation = queryset.query.annotations.get('config_context_data') + self.assertIsNotNone(config_annotation) + + def find_tag_subqueries(where_node): + """Find subqueries in WHERE clause that relate to tag filtering""" + subqueries = [] + + def traverse(node): + if hasattr(node, 'children'): + for child in node.children: + try: + if child.rhs.query.model is TaggedItem: + subqueries.append(child.rhs.query) + except AttributeError: + traverse(child) + traverse(where_node) + return subqueries + + # Find subqueries in the WHERE clause that should have DISTINCT + tag_subqueries = find_tag_subqueries(config_annotation.query.where) + distinct_subqueries = [sq for sq in tag_subqueries if sq.distinct] + + # Verify we found at least one DISTINCT subquery for tags + self.assertEqual(len(distinct_subqueries), 1) + self.assertTrue(distinct_subqueries[0].distinct) class ConfigTemplateTest(TestCase):