From 3c2065c89da757406f080d9252e2c3019c66e8c7 Mon Sep 17 00:00:00 2001 From: Raphael Collet Date: Wed, 5 Apr 2017 10:48:39 +0200 Subject: [PATCH 1/2] [FIX] expression: use sub-select when searching on many2many field Avoid pathological performance issue caused by injecting ids retrieved with another query. Consider a domain like `[('m2m', 'in', ids)]` on a many2many field. The current implementation will perform the subquery: SELECT m2m_id1 FROM m2m_table WHERE m2m_id2 IN (ids) and inject its result into the main query as: SELECT id FROM ... WHERE id IN (result_ids) The latter may be very slow if `result_ids` is a huge list of ids. The fix injects the first query into the main query as: SELECT id FROM ... WHERE id IN ( SELECT m2m_id1 FROM m2m_table WHERE m2m_id2 IN (ids) ) As a result, the database will typically JOIN both tables, and avoid generating the whole list from the subquery. --- openerp/osv/expression.py | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/openerp/osv/expression.py b/openerp/osv/expression.py index 2be571e538c..b9e2a396a3a 100644 --- a/openerp/osv/expression.py +++ b/openerp/osv/expression.py @@ -994,15 +994,16 @@ class expression(object): rel_table, rel_id1, rel_id2 = column._sql_names(model) #FIXME if operator == 'child_of': - def _rec_convert(ids): - if comodel == model: - return ids - return select_from_where(cr, rel_id1, rel_table, rel_id2, ids, operator) - ids2 = to_ids(right, comodel, context) dom = child_of_domain('id', ids2, comodel) ids2 = comodel.search(cr, uid, dom, context=context) - push(create_substitution_leaf(leaf, ('id', 'in', _rec_convert(ids2)), model)) + if comodel == model: + push(create_substitution_leaf(leaf, ('id', 'in', ids2), model)) + else: + subquery = 'SELECT "%s" FROM "%s" WHERE "%s" IN %%s' % (rel_id1, rel_table, rel_id2) + # avoid flattening of argument in to_sql() + subquery = cr.mogrify(subquery, [tuple(ids2)]) + push(create_substitution_leaf(leaf, ('id', 'inselect', (subquery, [])), internal=True)) else: call_null_m2m = True if right is not False: @@ -1024,8 +1025,11 @@ class expression(object): operator = 'in' # operator changed because ids are directly related to main object else: call_null_m2m = False - m2m_op = 'not in' if operator in NEGATIVE_TERM_OPERATORS else 'in' - push(create_substitution_leaf(leaf, ('id', m2m_op, select_from_where(cr, rel_id1, rel_table, rel_id2, res_ids, operator) or [0]), model)) + subop = 'not inselect' if operator in NEGATIVE_TERM_OPERATORS else 'inselect' + subquery = 'SELECT "%s" FROM "%s" WHERE "%s" IN %%s' % (rel_id1, rel_table, rel_id2) + # avoid flattening of argument in to_sql() + subquery = cr.mogrify(subquery, [tuple(filter(None, res_ids))]) + push(create_substitution_leaf(leaf, ('id', subop, (subquery, [])), internal=True)) if call_null_m2m: m2m_op = 'in' if operator in NEGATIVE_TERM_OPERATORS else 'not in' From 6595cfdf0c4ea64fd9271a06107cfa90297f246d Mon Sep 17 00:00:00 2001 From: Raphael Collet Date: Thu, 6 Apr 2017 15:27:21 +0200 Subject: [PATCH 2/2] [FIX] expression: avoid useless query when searching on x2many sub-field Searching on a domain like `[('m2m.sub', operator, value)]` currently does something like: right_ids = comodel.search([('sub', operator, value)]).ids table_ids = model.search([('m2m', 'in', right_ids)]).ids and reduces the domain triple to `('id', 'in', table_ids)`. The domain triple can actually be reduced to `('m2m', 'in', right_ids)`. With this reduction, the search on the field `m2m` will be done as part of the main query. And this will also enable the optimization of the former fix! --- openerp/addons/base/tests/test_expression.py | 12 ++++++------ openerp/osv/expression.py | 3 +-- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/openerp/addons/base/tests/test_expression.py b/openerp/addons/base/tests/test_expression.py index ffa22cc252a..75ee10a508f 100644 --- a/openerp/addons/base/tests/test_expression.py +++ b/openerp/addons/base/tests/test_expression.py @@ -178,8 +178,8 @@ class test_expression(common.TransactionCase): self.assertEqual(set(partner_ids), set([p_aa]), "_auto_join off: ('bank_ids.name', 'like', '..'): incorrect result") # Test produced queries - self.assertEqual(len(self.query_list), 3, - "_auto_join off: ('bank_ids.name', 'like', '..') should produce 3 queries (1 in res_partner_bank, 2 on res_partner)") + self.assertEqual(len(self.query_list), 2, + "_auto_join off: ('bank_ids.name', 'like', '..') should produce 2 queries (1 in res_partner_bank, 1 on res_partner)") sql_query = self.query_list[0].get_sql() self.assertIn('res_partner_bank', sql_query[0], "_auto_join off: ('bank_ids.name', 'like', '..') first query incorrect main table") @@ -190,7 +190,7 @@ class test_expression(common.TransactionCase): self.assertEqual(set(['%' + name_test + '%']), set(sql_query[2]), "_auto_join off: ('bank_ids.name', 'like', '..') first query incorrect parameter") - sql_query = self.query_list[2].get_sql() + sql_query = self.query_list[1].get_sql() self.assertIn('res_partner', sql_query[0], "_auto_join off: ('bank_ids.name', 'like', '..') third query incorrect main table") self.assertIn('"res_partner"."id" in (%s)', sql_query[1], @@ -205,8 +205,8 @@ class test_expression(common.TransactionCase): self.assertEqual(set(partner_ids), set([p_a, p_b]), "_auto_join off: ('child_ids.bank_ids.id', 'in', [..]): incorrect result") # Test produced queries - self.assertEqual(len(self.query_list), 5, - "_auto_join off: ('child_ids.bank_ids.id', 'in', [..]) should produce 5 queries (1 in res_partner_bank, 4 on res_partner)") + self.assertEqual(len(self.query_list), 3, + "_auto_join off: ('child_ids.bank_ids.id', 'in', [..]) should produce 3 queries (1 in res_partner_bank, 2 on res_partner)") # Do: one2many with _auto_join partner_bank_ids_col._auto_join = True @@ -437,7 +437,7 @@ class test_expression(common.TransactionCase): self.assertTrue(set([p_a, p_b]).issubset(set(partner_ids)), "_auto_join off: ('child_ids.state_id.country_id.code', 'like', '..') incorrect result") # Test produced queries - self.assertEqual(len(self.query_list), 5, + self.assertEqual(len(self.query_list), 4, "_auto_join off: ('child_ids.state_id.country_id.code', 'like', '..') number of queries incorrect") # Do: ('child_ids.state_id.country_id.code', 'like', '..') with _auto_join diff --git a/openerp/osv/expression.py b/openerp/osv/expression.py index b9e2a396a3a..235416dc08d 100644 --- a/openerp/osv/expression.py +++ b/openerp/osv/expression.py @@ -869,8 +869,7 @@ class expression(object): # Making search easier when there is a left operand as column.o2m or column.m2m elif len(path) > 1 and column and column._type in ['many2many', 'one2many']: right_ids = comodel.search(cr, uid, [(path[1], operator, right)], context=context) - table_ids = model.search(cr, uid, [(path[0], 'in', right_ids)], context=dict(context, active_test=False)) - leaf.leaf = ('id', 'in', table_ids) + leaf.leaf = (path[0], 'in', right_ids) push(leaf) elif not column: