Skip to content
Snippets Groups Projects

Executing StockMove assign_try concurrently quantity assigned is greater than available quantity

    • Closed Issue created by Marius Grigaitis @mar

    Using postgresql, trytond 3.4

    How to repeat it:
    1. Create two productions with many input moves (so that assignment doesn't happen immediately)
    2. Purchase enough quantity to fulfill one production
    3. Assign productions concurrently (one after another)

    Result: Both productions assigned
    Expected Result: One production failed to assign

    Expected result happens when assigning productions one by one, but not concurrently.

    To upload designs, you'll need to enable LFS and have an admin enable hashed storage. More information

    Child items ...

    • Activity

    • Marius Grigaitis added stock + 1 deleted label

      added stock + 1 deleted label

    • Sergi Almacellas Abellana
      Sergi Almacellas Abellana @pokoli ·
      Developer

      A more detailed explanation of the issue and proposed solutions can be found on

      http://www.tryton.org/~irclog/2014-11-27.log.html#t2014-11-27%2014:56

    • Sergi Almacellas Abellana added type::behavior + 1 deleted label and removed 1 deleted label

      added type::behavior + 1 deleted label and removed 1 deleted label

    • Marius Grigaitis
      Marius Grigaitis @mar ·
      Author

      Related: #2280 (closed)

      I believe changing lock mode introduced this bug.

    • Cédric Krier
      Cédric Krier @ced ·
      Owner

      For me, the issue comes from also the change to repeatable read instead of serializable.
      So I guess the only solution if we want to keep this isolation level, is to use a SELECT ... FOR UPDATE on move having the products.
      But it is only supported on postgresql.

    • Marius Grigaitis
      Marius Grigaitis @mar ·
      Author

      I've tried adding logging into assign_try.

      Tried 2 scenarios, with 2 windows open:

      1) Clicking assign on production #1, waiting for it to finish, clicking assign on production #2 (in another window), waiting for it to finish (actually fail because of not enough quantity). Success scenario
      2) Doing the same thing, but not waiting for #1 to finish. Fail scenario, all assignments succeeded.

      Log from 1):

      [Mon Dec 01 09:47:25 2014] INFO:assign_try:139656558614272 -- STOCK START ASSIGN
      [Mon Dec 01 09:47:25 2014] INFO:assign_try:139656558614272 -- PBL: {(9, 2689): 13.0, (9, 4565): 8.0, (9, 12746): 2.0, (9, 12747): 0.0, (9, 2674): 82.60000000000001, (9, 2675): 140.0}
      [Mon Dec 01 09:47:27 2014] INFO:assign_try:139656558614272 -- STOCK STOP ASSIGN
      [Mon Dec 01 09:47:31 2014] INFO:assign_try:139656541566720 -- STOCK START ASSIGN
      [Mon Dec 01 09:47:31 2014] INFO:assign_try:139656541566720 -- PBL: {(9, 2689): 0.0, (9, 4565): 4.0, (9, 12746): 1.0, (9, 12747): 0.0, (9, 2674): 76.60000000000001, (9, 2675): 134.5}
      [Mon Dec 01 09:47:32 2014] INFO:assign_try:139656541566720 -- STOCK STOP ASSIGN


      Log from 2)

      [Mon Dec 01 09:48:52 2014] INFO:assign_try:139981410346752 -- STOCK START ASSIGN
      [Mon Dec 01 09:48:52 2014] INFO:assign_try:139981410346752 -- PBL: {(9, 2689): 13.0, (9, 4565): 8.0, (9, 12746): 2.0, (9, 12747): 0.0, (9, 2674): 82.60000000000001, (9, 2675): 140.0}
      [Mon Dec 01 09:48:53 2014] INFO:assign_try:139981410346752 -- STOCK STOP ASSIGN
      [Mon Dec 01 09:48:54 2014] INFO:assign_try:139981401954048 -- STOCK START ASSIGN
      [Mon Dec 01 09:48:54 2014] INFO:assign_try:139981401954048 -- PBL: {(9, 2689): 13.0, (9, 4565): 8.0, (9, 12746): 2.0, (9, 12747): 0.0, (9, 2674): 82.60000000000001, (9, 2675): 140.0}
      [Mon Dec 01 09:48:55 2014] INFO:assign_try:139981401954048 -- STOCK STOP ASSIGN

      STOCK START ASSIGN is just after .lock()
      STOCK STOP is at the end for assign_try.
      PBL is output of pbl variable just after .products_by_location()

    • Mathias Behrle
      Mathias Behrle @mbehrle ·
      Developer

      AFAIS MySQL should support SELECT ... FOR UPDATE at least since 4.1, too.

      https://dev.mysql.com/doc/refman/4.1/en/innodb-locking-reads.html

    • Marius Grigaitis
      Marius Grigaitis @mar ·
      Author

      I tried simple solutions like adding FOR UPDATE into products by location query,

      1) Tried adding FOR UPDATE in the main query, removed group by:
      NotSupportedError: FOR UPDATE is not allowed with UNION/INTERSECT/EXCEPT

      2) Tried adding FOR UPDATE into inside queries:
      Got syntax error "FOR UPDATE UNION". Looks like UNION + FOR UPDATE doesn't go together

      Also FOR UPDATE + GROUP BY throws exception, because PostgreSQL is unable to do FOR UPDATE + GROUP BY.

    • Marius Grigaitis
      Marius Grigaitis @mar ·
      Author

      NotSupportedError: FOR UPDATE is not allowed with GROUP BY clause

    • Marius Grigaitis
      Marius Grigaitis @mar ·
      Author

      I was unable to repeat the issue with conn.set_isolation_level(ISOLATION_LEVEL_SERIALIZABLE)

      http://hg.tryton.org/trytond/changeset/0a4097cbe662

    • Marius Grigaitis
      Marius Grigaitis @mar ·
      Author

      Added revert for isolation level change

      review9931002

    • Cédric Krier
      Cédric Krier @ced ·
      Owner

      I don't agree to go back to serializable level, it cost too much.
      SELECT ... FOR UPDATE must be added to assign_try.

    • Cédric Krier
      Cédric Krier @ced ·
      Owner

      Indeed, an easier way is to not wait for lock in other transactions.
      This review5901002 will be easier to backport and is also probably better than "SELECT ... FOR UPDATE" as it will require less write on disk.

    • Cédric Krier assigned to @ced

      assigned to @ced

    • Cédric Krier added 1 deleted label and removed 1 deleted label

      added 1 deleted label and removed 1 deleted label

    • Albert Cervera
      Albert Cervera @albertca ·
      Developer

      Not waiting for other transactions is likely to end up with the application crashing to the user so I don't think that's a good solution either.

      There's been the patch with advisory locks for some time review999002 which would also reduce contention. Although it is only available for PostgreSQL. Other databases could use a standard lock table.

      Another option which would be quite simple would be to add the possibility to define the isolation level with RPC(serializable=True) and pass that to database backends. The main problem with this is that it would only be possible to make assign_try() work correctly if the transaction is started with serializable so anyone calling assign_try() elsewhere (because they're automating from a write() call or something like that) would have some problems.

    • Cédric Krier
      Cédric Krier @ced ·
      Owner

      On 22 Dec 01:04, Albert Cervera i Areny wrote:
      > Not waiting for other transactions is likely to end up with the application crashing to the user so I don't think that's a good solution either.

      Yes and that's exactly what we want: retry until we get a serializable
      (over stock_move table) transaction and if we can not then it must fail.

      > There's been the patch with advisory locks for some time review999002 which would also reduce contention. Although it is only available for PostgreSQL. Other databases could use a standard lock table.

      This doesn't help because it just return true/false and it doesn't
      restart the all transaction.

      > Another option which would be quite simple would be to add the possibility to define the isolation level with RPC(serializable=True) and pass that to database backends. The main problem with this is that it would only be possible to make assign_try() work correctly if the transaction is started with serializable so anyone calling assign_try() elsewhere (because they're automating from a write() call or something like that) would have some problems.

      -1

    • Roundup Robot
      Roundup Robot @roundup-bot ·

      New changeset eba810510a4a by C?dric Krier in branch 'default':
      Use NOWAIT when locking a table
      http://hg.tryton.org/trytond/rev/eba810510a4a

    • Roundup Robot added 1 deleted label and removed 1 deleted label

      added 1 deleted label and removed 1 deleted label

    • Roundup Robot closed

      closed

    • Roundup Robot
      Roundup Robot @roundup-bot ·

      New changeset ee0dc68b8370 by C?dric Krier in branch '3.4':
      Use NOWAIT when locking a table
      http://hg.tryton.org/trytond/rev/ee0dc68b8370

      New changeset ff18d9bb3634 by C?dric Krier in branch '3.2':
      Use NOWAIT when locking a table
      http://hg.tryton.org/trytond/rev/ff18d9bb3634

      New changeset 50d6fba239af by C?dric Krier in branch '3.0':
      Use NOWAIT when locking a table
      http://hg.tryton.org/trytond/rev/50d6fba239af

      New changeset 8d718e5e10fc by C?dric Krier in branch '2.8':
      Use NOWAIT when locking a table
      http://hg.tryton.org/trytond/rev/8d718e5e10fc

      New changeset 6d6acefd867d by C?dric Krier in branch '2.6':
      Use NOWAIT when locking a table
      http://hg.tryton.org/trytond/rev/6d6acefd867d

    Please register or sign in to reply