diff --git a/packages/serveradmin/serveradmin/serverdb/sql_generator.py b/packages/serveradmin/serveradmin/serverdb/sql_generator.py index 039298ef..a0177ddc 100644 --- a/packages/serveradmin/serveradmin/serverdb/sql_generator.py +++ b/packages/serveradmin/serveradmin/serverdb/sql_generator.py @@ -271,56 +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 = [] + attribute_conditions = ( + "sub.attribute_id = '{0}'".format(attribute.attribute_id), + template.format('sub.value'), + ) + + # 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: - # 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' - ), + condition = _exists_sql( + model, 'sub', + ('server.server_id = sub.server_id',) + attribute_conditions, ) - 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)) - - 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) + condition = '(server.server_id IN ({0}))'.format( + _inherited_server_ids_sql( + model, attribute_conditions, related_via_attribute + ) ) - for relation_condition, servertype_ids in relation_conditions - )) - - return _exists_sql(model, 'sub', ( - mixed_relation_condition, - "sub.attribute_id = '{0}'".format(attribute.attribute_id), - template.format('sub.value'), + path_conditions.append((condition, servertype_ids)) + + if len(path_conditions) == 1: + return path_conditions[0][0] + + # 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 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), condition + ) + for condition, servertype_ids in path_conditions )) @@ -330,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 00000000..1fc20456 --- /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'}) 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 e014be07..d40ad6fc 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.