From bd6f0136c380dd8f35cbb0574a106d3607282604 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Kr=C3=B6ger?= Date: Fri, 18 Sep 2026 09:43:49 +0000 Subject: [PATCH 1/2] perf(serverdb): emit one EXISTS per relation path in filters When an attribute is attached directly to some servertypes and inherited by others through a related_via attribute, _real_condition_sql() built a single EXISTS over the value table whose WHERE OR'ed the relation paths together. Postgres can turn a correlated EXISTS with one path into a hash semi join, but not one whose correlation to "server" is an OR of alternatives: it fell back to a nested loop over every (server, sub) pair and evaluated the inherited path as a sub plan for each of them. Emit one EXISTS per path instead and OR them outside, each guarded by its servertype test written first. The guard order matters: Postgres reorders AND clauses by cost at the top level only, not inside the branches of an OR, and otherwise evaluates left to right, so the cheap test now short-circuits the EXISTS for servertypes that do not use that path. Boolean-equivalent to the old form, since the guards do not depend on the inner row and factor out of the existential. --- .../serveradmin/serverdb/sql_generator.py | 42 +++++++++++++------ .../serverdb/tests/test_ip_addr_type.py | 27 ++++++++++++ 2 files changed, 56 insertions(+), 13 deletions(-) diff --git a/packages/serveradmin/serveradmin/serverdb/sql_generator.py b/packages/serveradmin/serveradmin/serverdb/sql_generator.py index 039298efb..e8dd2a01d 100644 --- a/packages/serveradmin/serveradmin/serverdb/sql_generator.py +++ b/packages/serveradmin/serveradmin/serverdb/sql_generator.py @@ -306,21 +306,37 @@ def _real_condition_sql(attribute, template, related_vias): )) relation_conditions.append((relation_condition, servertype_ids)) - if len(relation_conditions) == 1: - mixed_relation_condition = relation_conditions[0][0] - else: - mixed_relation_condition = '({0})'.format(' OR '.join( - '({0} AND server.servertype_id IN ({1}))' - .format(relation_condition, ', '.join( - "'{0}'".format(s) for s in servertype_ids) - ) - for relation_condition, servertype_ids in relation_conditions - )) - - return _exists_sql(model, 'sub', ( - mixed_relation_condition, + attribute_conditions = ( "sub.attribute_id = '{0}'".format(attribute.attribute_id), template.format('sub.value'), + ) + + if len(relation_conditions) == 1: + return _exists_sql( + model, 'sub', (relation_conditions[0][0],) + attribute_conditions + ) + + # One EXISTS per relation path, OR'ed together outside them, rather than + # a single EXISTS with the paths OR'ed inside its WHERE. Postgres can + # turn a correlated EXISTS with one path into a hash semi join, but not + # one whose correlation to "server" is an OR of alternatives: that + # degrades to a nested loop over every (server, sub) pair, with the + # inherited paths evaluated as a sub plan per pair - millions of + # executions for a filter that matches a few hundred rows. + # + # The servertype guard comes first in each branch on purpose. Postgres + # reorders AND clauses by cost at the top level only, not inside the + # branches of an OR, and otherwise evaluates left to right; the cheap + # test first lets it skip the EXISTS for servertypes that do not use + # that path at all. + return '({0})'.format(' OR '.join( + '(server.servertype_id IN ({0}) AND {1})'.format( + ', '.join("'{0}'".format(s) for s in servertype_ids), + _exists_sql( + model, 'sub', (relation_condition,) + attribute_conditions + ), + ) + for relation_condition, servertype_ids in relation_conditions )) diff --git a/packages/serveradmin/serveradmin/serverdb/tests/test_ip_addr_type.py b/packages/serveradmin/serveradmin/serverdb/tests/test_ip_addr_type.py index e014be07c..d40ad6fca 100644 --- a/packages/serveradmin/serveradmin/serverdb/tests/test_ip_addr_type.py +++ b/packages/serveradmin/serveradmin/serverdb/tests/test_ip_addr_type.py @@ -788,6 +788,33 @@ def test_af_unaware_supernet_related(self): (self.rn_ipv4["hostname"], self.rn_ipv6["hostname"]), ) + def test_related_attribute_without_servertype_filter(self): + # Filtering on an attribute that some servertypes carry directly and + # others inherit, without narrowing the servertype, makes the SQL + # generator emit one EXISTS per relation path (see + # sql_generator._real_condition_sql). Both paths have to deliver: + # the route networks match directly, server_rn through its + # AF-unaware supernet. server_pn sits in no route network. + hostnames = { + server["hostname"] + for server in Query( + { + "network_type": filters.Any( + "internal_ipv4", "internal_ipv6" + ), + }, + ["hostname"], + ) + } + self.assertEqual( + hostnames, + { + self.rn_ipv4["hostname"], + self.rn_ipv6["hostname"], + self.server_rn["hostname"], + }, + ) + def test_af_aware_supernet(self): # Querying for AF-aware supernet attribute will find only objects # matching the given address family. From 5817e1505cc9e04f56b8fe58eaf912e8493137c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Kr=C3=B6ger?= Date: Fri, 18 Sep 2026 10:28:04 +0000 Subject: [PATCH 2/2] perf(serverdb): render inherited filter paths as uncorrelated subqueries An attribute inherited through a related_via attribute was filtered with an EXISTS correlated to the outer server row. Postgres evaluated it once per candidate server, re-finding the same matching rows every time and then testing each (server, match) pair one by one. Render each inherited path as "server.server_id IN (subquery)" with nothing in the subquery referencing the outer server. Postgres then evaluates it once, hashes the ids and probes per row, and for a supernet path drives the containment join from the matching networks' prefixes, which is what the GiST index exists for. Cost becomes (candidates + matches) rather than their product. The directly attached path keeps its correlated EXISTS, which Postgres already turns into a semi join (or an anti join under NOT), so the SQL for attributes that are not inherited anywhere is unchanged. The selected column is a NOT NULL foreign key in every branch, so NOT IN keeps set semantics: no NULL can make the test unknown. _supernet_af_sql() factors the address-family joins out of _supernet_exists_sql(), which still serves direct supernet filters and ContainedOnlyBy and emits exactly what it did before. --- .../serveradmin/serverdb/sql_generator.py | 188 ++++++++++++------ .../serverdb/tests/test_inherited_filters.py | 67 +++++++ 2 files changed, 190 insertions(+), 65 deletions(-) create mode 100644 packages/serveradmin/serveradmin/serverdb/tests/test_inherited_filters.py diff --git a/packages/serveradmin/serveradmin/serverdb/sql_generator.py b/packages/serveradmin/serveradmin/serverdb/sql_generator.py index e8dd2a01d..a0177ddc6 100644 --- a/packages/serveradmin/serveradmin/serverdb/sql_generator.py +++ b/packages/serveradmin/serveradmin/serverdb/sql_generator.py @@ -271,72 +271,49 @@ def _real_condition_sql(attribute, template, related_vias): related_vias = related_vias[attribute.attribute_id] assert related_vias - # We start with the condition for the attributes the server has on - # its own. Then, add the conditions for all possible relations. They - # are going to be OR'ed together. - relation_conditions = [] - for related_via_attribute, servertype_ids in related_vias.items(): - if related_via_attribute is None: - # The condition for directly attached attributes - relation_condition = 'server.server_id = sub.server_id' - elif related_via_attribute.type == 'supernet': - relation_condition = _supernet_exists_sql( - related_via_attribute, 'supernet', - '>>= server_addr.value', - ( - _target_servertype_sql('supernet', related_via_attribute), - 'supernet.server_id = sub.server_id' - ), - ) - elif related_via_attribute.type == 'reverse': - relation_condition = _exists_sql(ServerRelationAttribute, 'rel1', ( - "rel1.attribute_id = '{0}'".format( - related_via_attribute.reversed_attribute_id - ), - 'rel1.value = server.server_id', - 'rel1.server_id = sub.server_id', - )) - else: - assert related_via_attribute.type == 'relation' - relation_condition = _exists_sql(ServerRelationAttribute, 'rel1', ( - "rel1.attribute_id = '{0}'" - .format(related_via_attribute.attribute_id), - 'rel1.server_id = server.server_id', - 'rel1.value = sub.server_id', - )) - relation_conditions.append((relation_condition, servertype_ids)) - attribute_conditions = ( "sub.attribute_id = '{0}'".format(attribute.attribute_id), template.format('sub.value'), ) - if len(relation_conditions) == 1: - return _exists_sql( - model, 'sub', (relation_conditions[0][0],) + attribute_conditions - ) + # One condition per relation path. The directly attached path stays a + # correlated EXISTS: with a single equality to "server" Postgres turns + # it into a hash semi join, or an anti join under NOT. The inherited + # paths are deliberately *uncorrelated* subqueries instead - see + # _inherited_server_ids_sql() for why. + path_conditions = [] + for related_via_attribute, servertype_ids in related_vias.items(): + if related_via_attribute is None: + condition = _exists_sql( + model, 'sub', + ('server.server_id = sub.server_id',) + attribute_conditions, + ) + else: + condition = '(server.server_id IN ({0}))'.format( + _inherited_server_ids_sql( + model, attribute_conditions, related_via_attribute + ) + ) + path_conditions.append((condition, servertype_ids)) + + if len(path_conditions) == 1: + return path_conditions[0][0] - # One EXISTS per relation path, OR'ed together outside them, rather than - # a single EXISTS with the paths OR'ed inside its WHERE. Postgres can - # turn a correlated EXISTS with one path into a hash semi join, but not - # one whose correlation to "server" is an OR of alternatives: that - # degrades to a nested loop over every (server, sub) pair, with the - # inherited paths evaluated as a sub plan per pair - millions of - # executions for a filter that matches a few hundred rows. + # The paths are OR'ed together outside their subqueries, rather than + # inside a single EXISTS: Postgres cannot make a semi join out of an + # EXISTS whose correlation to "server" is an OR of alternatives, and + # falls back to a nested loop over every (server, sub) pair. # # The servertype guard comes first in each branch on purpose. Postgres # reorders AND clauses by cost at the top level only, not inside the # branches of an OR, and otherwise evaluates left to right; the cheap - # test first lets it skip the EXISTS for servertypes that do not use - # that path at all. + # test first lets it skip the subquery probe for servertypes that do + # not use that path at all. return '({0})'.format(' OR '.join( '(server.servertype_id IN ({0}) AND {1})'.format( - ', '.join("'{0}'".format(s) for s in servertype_ids), - _exists_sql( - model, 'sub', (relation_condition,) + attribute_conditions - ), + ', '.join("'{0}'".format(s) for s in servertype_ids), condition ) - for relation_condition, servertype_ids in relation_conditions + for condition, servertype_ids in path_conditions )) @@ -346,19 +323,100 @@ def _exists_sql(model, alias, conditions): ) -def _supernet_exists_sql(attribute: Attribute, supernet_alias: str, addr_match: str, where: tuple[str, ...]): - if attribute.inet_address_family: - af_join = ( - (Attribute._meta.db_table, 'server_attr', ('server_attr.attribute_id = server_addr.attribute_id',)), - (Attribute._meta.db_table, 'net_attr', ('net_attr.attribute_id = net_addr.attribute_id',)), - ) - af_where = ( - f"net_attr.inet_address_family = '{attribute.inet_address_family}'", - f"server_attr.inet_address_family = '{attribute.inet_address_family}'", - ) +def _inherited_server_ids_sql( + model, attribute_conditions, related_via_attribute +): + """SELECT the ids of servers inheriting a matching value via an attribute + + Nothing in the returned subquery references the outer "server". That is + the point: as a correlated EXISTS, an inherited path was re-evaluated + once per candidate server, re-finding the same matching rows every time + and then testing each (server, match) pair one by one - for a supernet + path that meant millions of inet containment checks, none of them able + to use the GiST index on server_inet_attribute.value because the plan + probed the server's address by key and only then filtered on + containment. Uncorrelated, Postgres evaluates it once, hashes the ids, + and probes the hash per row; the containment join is driven from the + matching networks' prefixes, which is what the index is for. The cost + becomes (candidates + matches) instead of their product. + + The selected column is a NOT NULL foreign key in every branch, so the + caller's "server.server_id IN (...)" keeps plain set semantics under NOT + as well: no NULL can leak into the result and turn the test unknown. + """ + sub_table = model._meta.db_table + rel_table = ServerRelationAttribute._meta.db_table + conditions = list(attribute_conditions) + + if related_via_attribute.type == 'relation': + # The server points at the owner of the value. + select = 'rel1.server_id' + from_ = '{0} AS rel1'.format(rel_table) + joins = [ + 'JOIN {0} AS sub ON (sub.server_id = rel1.value)'.format(sub_table) + ] + conditions.insert(0, "rel1.attribute_id = '{0}'".format( + related_via_attribute.attribute_id + )) + elif related_via_attribute.type == 'reverse': + # The owner of the value points at the server. + select = 'rel1.value' + from_ = '{0} AS rel1'.format(rel_table) + joins = [ + 'JOIN {0} AS sub ON (sub.server_id = rel1.server_id)' + .format(sub_table) + ] + conditions.insert(0, "rel1.attribute_id = '{0}'".format( + related_via_attribute.reversed_attribute_id + )) else: - af_join = () - af_where = () + assert related_via_attribute.type == 'supernet' + # The owner of the value is a network containing one of the + # server's addresses on the same inet attribute. Same join graph + # as _supernet_exists_sql(), minus the correlation to "server". + af_join, af_where = _supernet_af_sql(related_via_attribute) + select = 'server_addr.server_id' + from_ = '{0} AS sub'.format(sub_table) + joins = [ + 'JOIN {0} AS supernet ON (supernet.server_id = sub.server_id)' + .format(Server._meta.db_table), + 'JOIN {0} AS net_addr ON (net_addr.server_id = supernet.server_id)' + .format(ServerInetAttribute._meta.db_table), + 'JOIN {0} AS server_addr ON (' + 'server_addr.attribute_id = net_addr.attribute_id' + ' AND net_addr.value >>= server_addr.value)' + .format(ServerInetAttribute._meta.db_table), + ] + [ + f'JOIN {x[0]} AS {x[1]} ON ({" AND ".join(x[2])})' for x in af_join + ] + conditions.insert( + 0, _target_servertype_sql('supernet', related_via_attribute) + ) + conditions.extend(af_where) + + return 'SELECT {0} FROM {1} {2} WHERE {3}'.format( + select, from_, ' '.join(joins), + ' AND '.join(c for c in conditions if c), + ) + + +def _supernet_af_sql(attribute): + """Joins and conditions pinning a supernet match to one address family""" + if not attribute.inet_address_family: + return (), () + af_join = ( + (Attribute._meta.db_table, 'server_attr', ('server_attr.attribute_id = server_addr.attribute_id',)), + (Attribute._meta.db_table, 'net_attr', ('net_attr.attribute_id = net_addr.attribute_id',)), + ) + af_where = ( + f"net_attr.inet_address_family = '{attribute.inet_address_family}'", + f"server_attr.inet_address_family = '{attribute.inet_address_family}'", + ) + return af_join, af_where + + +def _supernet_exists_sql(attribute: Attribute, supernet_alias: str, addr_match: str, where: tuple[str, ...]): + af_join, af_where = _supernet_af_sql(attribute) joins = ( (ServerInetAttribute._meta.db_table, 'server_addr', ('server_addr.server_id = server.server_id',)), diff --git a/packages/serveradmin/serveradmin/serverdb/tests/test_inherited_filters.py b/packages/serveradmin/serveradmin/serverdb/tests/test_inherited_filters.py new file mode 100644 index 000000000..1fc204561 --- /dev/null +++ b/packages/serveradmin/serveradmin/serverdb/tests/test_inherited_filters.py @@ -0,0 +1,67 @@ +"""Serveradmin - Filters on inherited attributes + +Copyright (c) 2026 InnoGames GmbH +""" + +from django.contrib.auth.models import User +from django.test import TransactionTestCase + +from adminapi.filters import Not +from serveradmin.dataset import Query +from serveradmin.serverdb.models import ServertypeAttribute + + +class InheritedFilterTest(TransactionTestCase): + """Filter on an attribute that some servertypes inherit + + The fixture has vm-1 pointing at hv-1 through the "hypervisor" relation + attribute, with "vms" as its reverse, and neither servertype carrying + "os". Each test wires "os" onto one of them directly and onto the other + through a relation path, then filters on it without narrowing the + servertype, so the SQL generator has to render both the direct and the + inherited path (see sql_generator._inherited_server_ids_sql). The + supernet path is covered in test_ip_addr_type. + """ + + fixtures = ['auth_user.json', 'test_dataset.json'] + + def _set_os(self, hostname, value): + query = Query({'hostname': hostname}, ['os']) + query.update(os=value) + query.commit(user=User.objects.first()) + + def _hostnames(self, os_filter): + return {s['hostname'] for s in Query({'os': os_filter}, ['hostname'])} + + def test_inherited_via_relation(self): + # hypervisor carries "os", vm inherits it from its hypervisor. + ServertypeAttribute.objects.create( + servertype_id='hypervisor', attribute_id='os', + ) + ServertypeAttribute.objects.create( + servertype_id='vm', attribute_id='os', + related_via_attribute_id='hypervisor', + ) + self._set_os('hv-1', 'buster') + + self.assertEqual(self._hostnames('buster'), {'hv-1', 'vm-1'}) + + # The inheriting server must drop out under negation too: the + # subquery selects a NOT NULL column, so NOT IN keeps set semantics. + excluded = self._hostnames(Not('buster')) + self.assertNotIn('hv-1', excluded) + self.assertNotIn('vm-1', excluded) + self.assertIn('test0', excluded) + + def test_inherited_via_reverse(self): + # vm carries "os", hypervisor inherits it from its vms. + ServertypeAttribute.objects.create( + servertype_id='vm', attribute_id='os', + ) + ServertypeAttribute.objects.create( + servertype_id='hypervisor', attribute_id='os', + related_via_attribute_id='vms', + ) + self._set_os('vm-1', 'buster') + + self.assertEqual(self._hostnames('buster'), {'vm-1', 'hv-1'})