From 313d7071a9580ac401f58369b4c834eb2018f0ed Mon Sep 17 00:00:00 2001 From: william-andre Date: Thu, 1 Feb 2024 14:05:22 +0100 Subject: [PATCH] [IMP] account: reduce number of queries for reconciliation Summary: ======== 1. Handle the inverse fields of `account.full.reconcile` manually 2. Batch the update of matching numbers 3. Batch the balance consistency test 4. Batch the construction of reconciliation batches 5. Precompute fields on partials 6. Avoid recomputation of same default analytic distributions 7. Cache the accounting date wrt lock date 1: Handle the inverse fields of `account.full.reconcile` manually ----------------------------------------------------------------- In order to update the inverse fields of `reconciled_line_ids` and `partial_reconcile_ids`, the ORM is triggering one query per full reconcile record. In the case of a big batch payment, that is a lot of round trips. This also bypasses the computation for possible tracking, which is expensive to do for 2 records at a time. That field is not tracked anyway. 2: Batch the update of matching numbers --------------------------------------- Basically the same as 1. without the tracking. 3: Batch the balance consistency test ------------------------------------- Use the various context managers around `__reconcile_plan` to check for the balance consistency only at the end in one single query. This avoids checking it at every single intermediate write done on the lines. Also doing it for the sync of dynamic lines. It should be useless but it doesn't cost anything, and allows to remove the context keys in enterprise without a cost. (they shouldn't be used explicitly, the context managers should always be used instead) Using these context managers also allows to remove all the `skip_` context keys, which should not be used: using them will mean that as soon as the journal entry is modified for any reason, some business fields will change because of the synchronisation since the context won't be the same anymore. This commit therefore also makes `_sync_dynamic_line` more resilient and avoids even more useless syncs. 4: Batch the construction of reconciliation batches --------------------------------------------------- Introduce `_reconciled_by_number`, which is basically a `_read_group`. In conjuction with `_filter_reconciled_by_number`, we can find all the reconciled lines in one single query, for as many batches as needed; as opposed to one query per batch before. 5: Precompute fields on partials -------------------------------- Simply avoid doing one batched `INSERT INTO` followed by one `UPDATE` for each record. 6: Avoid recomputation of same default analytic distributions ------------------------------------------------------------- Not really linked to the reconciliation, but when a lot of lines are using the same parameters for the default analytic distribution, it is useless to re-compute it multiple times. Can speed up the population of test data for instance. 7: Cache the accounting date wrt lock date ------------------------------------------ When preparing the create values for the exchange moves, computing the date with regards to the lock date once per move is very expensive. By creating a context dependent field, we can cache the value and avoid doing the same operation multiple times. Benchmark: ========== We are testing 2 use cases: * a lot of small matchings that are producing a full 2 by 2 * one big matching (also producing a full) To reconcile a batch payment of 500 (identical) lines in the bank reco: | | Queries | SQL | Other | |--------|---------|-------|--------| | Before | 4191 | 4.649 | 16.253 | | After | 226 | 0.989 | 3.581 | To reconcile one big batch of 1000 lines with the generic reco widget (spaghetti): | | Queries | SQL | Other | |--------|---------|-------|-------| | Before | 82 | 0.478 | 1.131 | | After | 66 | 0.634 | 0.974 | To reconcile a batch of 1000 lines *in foreign currency* | | Queries | SQL | Other | |--------|---------|-------|-------| | Before | | | | | After | 45995 | 125.0 | 146.3 | All the times are in seconds. We can see the the use case of the batch payment is improved a lot (the time is expected to be linear with regards to the number of payments), while the time for big batches is not impacted. closes odoo/odoo#152284 Related: odoo/enterprise#55633 Signed-off-by: Brice Bartoletti (bib) --- .../account/models/account_full_reconcile.py | 36 ++++++++-- addons/account/models/account_journal.py | 11 +++ addons/account/models/account_move.py | 7 +- addons/account/models/account_move_line.py | 69 +++++++++++-------- .../models/account_partial_reconcile.py | 27 ++++---- .../account/wizard/account_move_reversal.py | 4 +- 6 files changed, 106 insertions(+), 48 deletions(-) diff --git a/addons/account/models/account_full_reconcile.py b/addons/account/models/account_full_reconcile.py index 559a7c2310f..519a7c0e78b 100644 --- a/addons/account/models/account_full_reconcile.py +++ b/addons/account/models/account_full_reconcile.py @@ -1,5 +1,5 @@ # -*- coding: utf-8 -*- -from odoo import api, fields, models, _ +from odoo import api, fields, models, _, Command class AccountFullReconcile(models.Model): @@ -37,7 +37,35 @@ class AccountFullReconcile(models.Model): @api.model_create_multi def create(self, vals_list): - fulls = super().create(vals_list) - for full in fulls.with_context(skip_matching_number_check=True): - full.reconciled_line_ids.matching_number = str(full.id) + def get_ids(commands): + for command in commands: + if command[0] == Command.LINK: + yield command[1] + elif command[0] == Command.SET: + yield from command[2] + else: + raise ValueError("Unexpected command: %s" % command) + move_line_ids = [list(get_ids(vals.pop('reconciled_line_ids'))) for vals in vals_list] + partial_ids = [list(get_ids(vals.pop('partial_reconcile_ids'))) for vals in vals_list] + fulls = super(AccountFullReconcile, self.with_context(tracking_disable=True)).create(vals_list) + + self.env.cr.execute_values(""" + UPDATE account_move_line line + SET full_reconcile_id = source.full_id + FROM (VALUES %s) AS source(full_id, line_ids) + WHERE line.id = ANY(source.line_ids) + """, [(full.id, line_ids) for full, line_ids in zip(fulls, move_line_ids)], page_size=1000) + fulls.reconciled_line_ids.invalidate_recordset(['full_reconcile_id'], flush=False) + fulls.invalidate_recordset(['reconciled_line_ids'], flush=False) + + self.env.cr.execute_values(""" + UPDATE account_partial_reconcile partial + SET full_reconcile_id = source.full_id + FROM (VALUES %s) AS source(full_id, partial_ids) + WHERE partial.id = ANY(source.partial_ids) + """, [(full.id, line_ids) for full, line_ids in zip(fulls, partial_ids)], page_size=1000) + fulls.partial_reconcile_ids.invalidate_recordset(['full_reconcile_id'], flush=False) + fulls.invalidate_recordset(['partial_reconcile_ids'], flush=False) + + self.env['account.partial.reconcile']._update_matching_number(fulls.reconciled_line_ids) return fulls diff --git a/addons/account/models/account_journal.py b/addons/account/models/account_journal.py index 5e54f7dd743..eec8ce0557b 100644 --- a/addons/account/models/account_journal.py +++ b/addons/account/models/account_journal.py @@ -207,6 +207,7 @@ class AccountJournal(models.Model): selected_payment_method_codes = fields.Char( compute='_compute_selected_payment_method_codes', ) + accounting_date = fields.Date(compute='_compute_accounting_date') _sql_constraints = [ ('code_company_uniq', 'unique (company_id, code)', 'Journal codes must be unique per company.'), @@ -355,6 +356,16 @@ class AccountJournal(models.Model): else: journal.suspense_account_id = False + @api.depends('company_id') + @api.depends_context('move_date', 'has_tax') + def _compute_accounting_date(self): + move_date = self.env.context.get('move_date') or fields.Date.context_today(self) + has_tax = self.env.context.get('has_tax') or False + for journal in self: + temp_move = self.env['account.move'].new({'journal_id': journal.id}) + journal.accounting_date = temp_move._get_accounting_date(move_date, has_tax) + + @api.onchange('type') def _onchange_type_for_alias(self): self.filtered(lambda journal: journal.type not in {'sale', 'purchase'}).alias_name = False diff --git a/addons/account/models/account_move.py b/addons/account/models/account_move.py index 7c7e58cf496..895c5aaab6f 100644 --- a/addons/account/models/account_move.py +++ b/addons/account/models/account_move.py @@ -2216,6 +2216,9 @@ class AccountMove(models.Model): dirty_recs = eligible_recs.filtered(dirty_fname) return dirty_recs, dirty_fname + def filter_trivial(mapping): + return {k: v for k, v in mapping.items() if 'id' not in k} + existing_before = existing() needed_before = needed() dirty_recs_before, dirty_fname = dirty() @@ -2241,7 +2244,9 @@ class AccountMove(models.Model): } if needed_after == needed_before: - return + return # do not modify user input if nothing changed in the needs + if not needed_before and (filter_trivial(existing_after) != filter_trivial(existing_before)): + return # do not modify user input if already created manually to_delete = [ line.id diff --git a/addons/account/models/account_move_line.py b/addons/account/models/account_move_line.py index 6497c2a6d94..30004f1f6ec 100644 --- a/addons/account/models/account_move_line.py +++ b/addons/account/models/account_move_line.py @@ -1119,9 +1119,10 @@ class AccountMoveLine(models.Model): @api.depends('account_id', 'partner_id', 'product_id') def _compute_analytic_distribution(self): + cache = {} for line in self: if line.display_type == 'product' or not line.move_id.is_invoice(include_receipts=True): - distribution = self.env['account.analytic.distribution.model']._get_distribution({ + arguments = frozendict({ "product_id": line.product_id.id, "product_categ_id": line.product_id.categ_id.id, "partner_id": line.partner_id.id, @@ -1129,7 +1130,9 @@ class AccountMoveLine(models.Model): "account_prefix": line.account_id.code, "company_id": line.company_id.id, }) - line.analytic_distribution = distribution or line.analytic_distribution + if arguments not in cache: + cache[arguments] = self.env['account.analytic.distribution.model']._get_distribution(arguments) + line.analytic_distribution = cache[arguments] or line.analytic_distribution @api.depends('discount_date', 'date_maturity') def _compute_payment_date(self): @@ -2333,12 +2336,17 @@ class AccountMoveLine(models.Model): :param reconciliation_plan: A list of reconciliation to perform. """ - # Parameter allowing to disable the exchange journal entries on partials. - disable_partial_exchange_diff = bool(self.env['ir.config_parameter'].sudo().get_param('account.disable_partial_exchange_diff')) - # ==== Prepare the reconciliation ==== # Batch the amls all together to know what should be reconciled and when. plan_list, all_amls = self._optimize_reconciliation_plan(reconciliation_plan) + move_container = {'records': all_amls.move_id} + with all_amls.move_id._check_balanced(move_container),\ + all_amls.move_id._sync_dynamic_lines(move_container): + self._reconcile_plan_with_sync(plan_list, all_amls) + + def _reconcile_plan_with_sync(self, plan_list, all_amls): + # Parameter allowing to disable the exchange journal entries on partials. + disable_partial_exchange_diff = bool(self.env['ir.config_parameter'].sudo().get_param('account.disable_partial_exchange_diff')) # ==== Prefetch the fields all at once to speedup the reconciliation ==== # All of those fields will be cached by the orm. Since the amls are split into multiple batches, the orm is not @@ -2423,12 +2431,13 @@ class AccountMoveLine(models.Model): full_batches = [] all_aml_ids = set() + number2lines = all_amls._reconciled_by_number() for plan in plan_list: for aml in plan['amls']: if 'full_batch_index' in aml_values_map[aml]: continue - involved_amls = plan['amls']._all_reconciled_lines() + involved_amls = plan['amls']._filter_reconciled_by_number(number2lines) all_aml_ids.update(involved_amls.ids) full_batch_index = len(full_batches) has_multiple_currencies = len(involved_amls.currency_id) > 1 @@ -2524,13 +2533,7 @@ class AccountMoveLine(models.Model): }) full_reconcile_full_batch_index.append(full_batch_index) - self.env['account.full.reconcile']\ - .with_context( - skip_invoice_sync=True, - skip_invoice_line_sync=True, - skip_account_move_synchronization=True, - )\ - .create(full_reconcile_values_list) + self.env['account.full.reconcile'].create(full_reconcile_values_list) # === Cash basis rounding autoreconciliation === # In case a cash basis rounding difference line got created for the transition account, we reconcile it with the corresponding lines @@ -2597,15 +2600,11 @@ class AccountMoveLine(models.Model): journal = company.currency_exchange_journal_id expense_exchange_account = company.expense_currency_exchange_account_id income_exchange_account = company.income_currency_exchange_account_id - - temp_exchange_move = self.env['account.move'].new({'journal_id': journal.id}) - accounting_exchange_date = temp_exchange_move._get_accounting_date( - exchange_date or fields.Date.context_today(self), - False, - ) + accounting_exchange_date = journal.with_context(move_date=exchange_date).accounting_date move_vals = { 'move_type': 'entry', + 'name': '/', # do not trigger the compute name before posting as it will most likely be posted immediately after 'date': accounting_exchange_date, 'journal_id': journal.id, 'line_ids': [], @@ -2711,13 +2710,7 @@ class AccountMoveLine(models.Model): )) # ==== Create the move ==== - exchange_moves = self.env['account.move']\ - .with_context( - skip_invoice_sync=True, - skip_invoice_line_sync=True, - skip_account_move_synchronization=True, - )\ - .create(exchange_move_values_list) + exchange_moves = self.env['account.move'].create(exchange_move_values_list) exchange_moves._post(soft=False) # ==== Reconcile ==== @@ -3060,12 +3053,28 @@ class AccountMoveLine(models.Model): ids.append(aml.id) return ids - def _all_reconciled_lines(self): - reconciled = self + def _reconciled_by_number(self) -> dict: + """Get the mapping of all the lines matched with the lines in self grouped by matching number.""" matching_numbers = [n for n in set(self.mapped('matching_number')) if n] if matching_numbers: - reconciled |= self.search([('matching_number', 'in', matching_numbers)]) - return reconciled + return dict(self._read_group( + domain=[('matching_number', 'in', matching_numbers)], + groupby=['matching_number'], + aggregates=['id:recordset'], + )) + return {} + + def _filter_reconciled_by_number(self, mapping: dict): + """Get all the the lines matched with the lines in self. + + Uses a mapping built with `_reconciled_by_number` to avoid multiple calls to the database. + """ + matching_numbers = [n for n in set(self.mapped('matching_number')) if n] + return self | self.browse([_id for number in matching_numbers for _id in mapping[number].ids]) + + def _all_reconciled_lines(self): + """Get all the the lines matched with the lines in self.""" + return self._filter_reconciled_by_number(self._reconciled_by_number()) def _get_attachment_domains(self): self.ensure_one() diff --git a/addons/account/models/account_partial_reconcile.py b/addons/account/models/account_partial_reconcile.py index b156d46dabf..171aa60af15 100644 --- a/addons/account/models/account_partial_reconcile.py +++ b/addons/account/models/account_partial_reconcile.py @@ -53,9 +53,11 @@ class AccountPartialReconcile(models.Model): company_id = fields.Many2one( comodel_name='res.company', string="Company", store=True, readonly=False, + precompute=True, compute='_compute_company_id') max_date = fields.Date( string="Max Date of Matched Lines", store=True, + precompute=True, compute='_compute_max_date') # used to determine at which date this reconciliation needs to be shown on the aged receivable/payable reports @@ -168,18 +170,19 @@ class AccountPartialReconcile(models.Model): line2number[partial.debit_move_id.id] = partial.id line2number[partial.credit_move_id.id] = partial.id - processed_aml_ids = [] - with amls.move_id._check_balanced({'records': amls.move_id}): # avoid checking the consistency for each individual write - for min_partial_id, line_ids in number2lines.items(): - min_partial = self.browse(min_partial_id) - lines = self.env['account.move.line'].browse(line_ids).with_context(skip_matching_number_check=True) - lines.matching_number = ( - str(min_partial.full_reconcile_id.id) - if min_partial.full_reconcile_id else - f"P{min_partial.id}" - ) - processed_aml_ids.extend(line_ids) - self.env['account.move.line'].browse(set(amls.ids) - set(processed_aml_ids)).matching_number = False + amls.flush_recordset(['full_reconcile_id']) + self.env.cr.execute_values(""" + UPDATE account_move_line l + SET matching_number = CASE + WHEN l.full_reconcile_id IS NOT NULL THEN l.full_reconcile_id::text + ELSE 'P' || source.number + END + FROM (VALUES %s) AS source(number, ids) + WHERE l.id = ANY(source.ids) + """, list(number2lines.items()), page_size=1000) + processed_amls = self.env['account.move.line'].browse([_id for ids in number2lines.values() for _id in ids]) + processed_amls.invalidate_recordset(['matching_number']) + (amls - processed_amls).matching_number = False # ------------------------------------------------------------------------- # RECONCILIATION METHODS diff --git a/addons/account/wizard/account_move_reversal.py b/addons/account/wizard/account_move_reversal.py index 2d20a38619c..25f1b771465 100644 --- a/addons/account/wizard/account_move_reversal.py +++ b/addons/account/wizard/account_move_reversal.py @@ -131,7 +131,9 @@ class AccountMoveReversal(models.TransientModel): if is_modify: moves_vals_list = [] for move in moves.with_context(include_business_fields=True): - moves_vals_list.append(move.copy_data({'date': self.date})[0]) + data = move.copy_data({'date': self.date})[0] + data['line_ids'] = [line for line in data['line_ids'] if line[2]['display_type'] == 'product'] + moves_vals_list.append(data) new_moves = self.env['account.move'].create(moves_vals_list) moves_to_redirect |= new_moves