[FIX] account: unique constraint in sequence mixin

[william-andre]

Change the way the uniqueness of the sequence numbers is ensured. Instead of using a SELECT FOR UPDATE approach inspired from the ir.sequence, we can use a constraint approach.

SELECT FOR UPDATE

By doing a FOR UPDATE NO WAIT, we are throwing an exception when another transaction is creating a move using the same sequence as this one, by locking the row that holds the current greatest sequence. Since the row doesn't change, the lock is released and the following UPDATE is allowed. Because of this, a "useless" UPDATE was always done on the previous row to ensure a SerializationFailureError if two concurrent transactions were trying to assign the same number. This means that in a database with a lot of concurrent transactions (typically with an online shop), many transactions were aborted/rollbacked.
This also approach also has the drawback of having the constraint implemented python side, which is less robust.

UNIQUE CONSTRAINT

Using a constraint on the database level means that the database knows when some transactions will try to do something similar. It will therefore make the concurrent transactions wait instead of aborting the transaction.
However, this approach can't lock only one sequence/journal, it locks the entire table, which can lead to a different kind of congestion. It should however always resolve and lead to a result without any transactions rollbacked.
It is also harder to have the constraint customized by different modules/localization because it is now done directly in the database and not in python. Changing the constraint means deleting/recreating the index. For now, this only happens for Latin America l10n, so a simple hardcoded approach is implemented.
In order to achieve this, the business code is trying to use the sequence incrementally until it gets a number it can use for when multiple transactions are concurrent.

[robodoo]

[poma-odoo]

It looks a bit scary to me, so if there is an exclusive locked for any reason, we keep increasing the sequence indefinitely?

[william-andre]

Why would you have an exclusive constraint on the sequence field apart from the one defined in this PR?

[poma-odoo]

I think, my code is the only one that does something, is not a valid assumption in a modular open-source software.
Why not increasing the number if we get UniqueViolation and retrying without increment when we have ExclusionViolation?

[william-andre]

Because UniqueViolation and ExclusionViolation are the same thing

[william-andre]

Stress test script. Tested locally with 6 concurrent executions.
There are no concurrent issues due to the sequences anymore.

import xmlrpc.client
import asyncio
import random

db = '16.0-sequence-mixin-constraint-wan'
username = 'admin'
password = 'admin'

common = xmlrpc.client.ServerProxy('http://localhost:8069/xmlrpc/2/common')
uid = common.authenticate(db, username, password, {})
models = xmlrpc.client.ServerProxy('http://localhost:8069/xmlrpc/2/object')

async def launch():
    invoice_id = models.execute_kw(db, uid, password, 'account.move', 'create', [{
        'move_type': 'out_invoice',
        'partner_id': random.randint(4, 100),
        'invoice_line_ids': [(0, 0, {'product_id': 4})],
    }])
    print('created invoice', invoice_id)
    models.execute_kw(db, uid, password, 'account.move', 'action_post', [invoice_id])

background_tasks = set()
async def main():
    while True:
        task = asyncio.create_task(launch())
        await asyncio.sleep(1)
        background_tasks.add(task)
        task.add_done_callback(background_tasks.discard)
asyncio.run(main())

[moylop260]

FYI deadlocks are still reproduced

I just migrated my unittest described from the issue #91873

• 16.0 - [REF] account: Adding unittests for concurrency issues in account_move sequences #104647
  • 

Also, there are other concurrency errors

Notice the v13.0 are not reproduced (using ir.sequence) the same unittest

• 13.0 - [REF] account: Adding unittests for concurrency issues in account_move sequences #91614

[jco-odoo]

Just an idea for the moment: Why not let the number not be set for a while when posted already and handle by a triggered cron afterwards asynchronously (only when it could not lock that cron)? #104661

[fpodoo]

I would raise an exception after 50 attemps, just in case, to avoid Python unlimited loops.

[odony]

Based on my limited testing, the deadlocks are directly caused by the EXCEPT constraint, which seems to introduce serialization cycles, even between only 2 transactions. The semantics of an EXCEPT contraints are a bit different from UNIQUE, and what we really want is a UNIQUE constraint, unfortunately we want it partial.

Another approach that seems to work is to only use a UNIQUE INDEX, not a full UNIQUE constraint. The index behaves like the constraint in terms of locking and ensuring uniqueness, but it works also when partial.

The following patch seems to eliminate the deadlock on my local setup. It also improves the locking performance for partner._increase_rank(). It's better to have a NO KEY lock to avoid blocking writer on other tables, and there are lots of FKs pointing to res_partner! This patch also fixes the flush_recordset() inside the savepoint: we have to flush also before the savepoint, otherwise we may rollback more than just the sequence field (e.g. the state too!), and that will never be flushed again. This was causing invoices to remain draft even after posting.

Of course this needs more cleanup, but it demonstrates the idea, hopefully.

diff --git addons/account/models/account_move.py addons/account/models/account_move.py
index a5f6f71a42ac..460d41bc4c53 100644
--- addons/account/models/account_move.py
+++ addons/account/models/account_move.py
@@ -552,6 +552,8 @@ class AccountMove(models.Model):
             ON account_move(journal_id) WHERE to_check = true;
             CREATE INDEX IF NOT EXISTS account_move_payment_idx
             ON account_move(journal_id, state, payment_state, move_type, date);
+            CREATE UNIQUE INDEX IF NOT EXISTS account_move_unique_name
+            ON account_move(name, journal_id) WHERE (state = 'posted' AND name != '/');
         """)
 
     # -------------------------------------------------------------------------
@@ -1593,12 +1595,6 @@ class AccountMove(models.Model):
     # CONSTRAINT METHODS
     # -------------------------------------------------------------------------
 
-    _sql_constraints = [(
-        'unique_name',
-        """EXCLUDE (name WITH =, journal_id WITH =) WHERE (state = 'posted' AND name != '/')""",
-        "Another entry with the same name already exists.",
-    )]
-
     @contextmanager
     def _check_balanced(self, container):
         ''' Assert the move is fully balanced debit = credit.
diff --git addons/account/models/partner.py addons/account/models/partner.py
index 4dfde347dc8d..19108323e0b9 100644
--- addons/account/models/partner.py
+++ addons/account/models/partner.py
@@ -4,7 +4,7 @@
 import time
 import logging
 
-from psycopg2 import sql, DatabaseError
+from psycopg2 import sql, errors
 
 from odoo import api, fields, models, _
 from odoo.osv import expression
@@ -650,11 +650,8 @@ class ResPartner(models.Model):
                     self.env.cr.execute(query, {'partner_ids': tuple(self.ids), 'n': n})
                     for partner in self:
                         self.env.cache.remove(partner, partner._fields[field])
-            except DatabaseError as e:
-                if e.pgcode == '55P03':
-                    _logger.debug('Another transaction already locked partner rows. Cannot update partner ranks.')
-                else:
-                    raise e
+            except (errors.LockNotAvailable, errors.SerializationFailure):
+                _logger.debug('Another transaction already locked partner rows. Cannot update partner ranks.')
 
     @api.model
     def get_partner_localisation_fields_required_to_invoice(self, country_id):
diff --git addons/account/models/sequence_mixin.py addons/account/models/sequence_mixin.py
index d369a59280fd..057cfb2a9ca6 100644
--- addons/account/models/sequence_mixin.py
+++ addons/account/models/sequence_mixin.py
@@ -176,7 +176,6 @@ class SequenceMixin(models.AbstractModel):
                 AND sequence_prefix = (SELECT sequence_prefix FROM {self._table} {where_string} ORDER BY id DESC LIMIT 1)
                 ORDER BY sequence_number DESC
                 LIMIT 1
-                {"FOR UPDATE" if lock else ""}
         """
 
         self.flush_model([self._sequence_field, 'sequence_number', 'sequence_prefix'])
@@ -240,6 +239,10 @@ class SequenceMixin(models.AbstractModel):
             format_values['year'] = self[self._sequence_date_field].year % (10 ** format_values['year_length'])
             format_values['month'] = self[self._sequence_date_field].month
 
+        # before flushing inside the savepoint (which may be rolled back!), make sure everything
+        # is already flushed, otherwise we could lose non-sequence fields values, as the ORM believes
+        # them to be flushed.
+        self.flush_recordset()
         while True:
             format_values['seq'] = format_values['seq'] + 1
             sequence = format.format(**format_values)

[moylop260]

FYI the last way 5d2f7ebf9b8f27c3afcc43a01b84aebda7121cbf is reducing the concurrency errors and the deadlock

Thank you!

However, there is a case of use not considered

It is the functional decision to configure one journal allowing gaps

e.g. Miscellaneous

In the past, we could decide to configure it as standard allowing gaps since that for some companies/countries we don't need to raise concurrency errors for a kind of document or we have an extra process to manage the GAPs (similar to SAP)

Using the new sequence way we are not able to decide it

Even if we have in the kanban a flag to identify if a journal has GAPs and control it from our side, the new way is forcing us to use standard for all journals without an extra option

In twitter or issues other people mentioned they are using standard instead of no-gaps for a particular country, company or journal

Will be this considered too?

[d-fence]

Upgrade exception #162 added.

Waiting the forward-port of this pr, the exception will be applied on all builds, and create an inconsistent state for migrations.
Please forward port this asap up to master without change. Exception should be forward-ported before the end of the week.

If you need to apply any change before it reaches master, please notify runbot team.

Details:

module:account_sequence

cc @KangOl @nseinlet

[KangOl]

Shouldn't we ensure the constraint exists?

[william-andre]

Yes, I changed it.
I'm not sure if it should be done in self.pool.post_init or self.pool.post_constraint though

[robodoo]

@william-andre @qdp-odoo staging failed: ci/l10n on 28d7162e1c552852b66c723d297ebaa4eefcaac5 (view more at https://runbot.odoo.com/runbot/build/21227672)

[qdp-odoo]

@robodoo r+ again please

[robodoo]

@william-andre @qdp-odoo staging failed: ci/l10n on a3bc4467858e11b2583bae90957a3dfce4311997 (view more at https://runbot.odoo.com/runbot/build/21232381)

[qdp-odoo]

@robodoo r+, maybe?

[qdp-odoo]

@robodoo r+ r+ r+ r+ r+ !!!!

[robodoo]

I'm sorry, @qdp-odoo:

• this PR is already reviewed, reviewing it again is useless.
• this PR is already reviewed, reviewing it again is useless.
• this PR is already reviewed, reviewing it again is useless.
• this PR is already reviewed, reviewing it again is useless.