Merge lp:~openerp-dev/openobject-addons/6.0-bug-770243-odo into lp:openobject-addons/6.0
- 6.0-bug-770243-odo
- Merge into 6.0
Proposed by
Olivier Dony (Odoo)
Status: | Merged |
---|---|
Merged at revision: | 4753 |
Proposed branch: | lp:~openerp-dev/openobject-addons/6.0-bug-770243-odo |
Merge into: | lp:openobject-addons/6.0 |
Diff against target: |
206 lines (+96/-84) 1 file modified
stock/stock.py (+96/-84) |
To merge this branch: | bzr merge lp:~openerp-dev/openobject-addons/6.0-bug-770243-odo |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
OpenERP Publisher's Warranty Team | Pending | ||
Review via email: mp+63116@code.launchpad.net |
Commit message
Description of the change
This branch fixes the performance issues that happen during reservation of stock.moves for databases with a large number of stock locations.
The locking and reservation system have been streamlined and are a lot more efficient in terms of iterations and database queries.
Support team should review and merge this for OPW.
Thanks!
To post a comment you must log in.
Revision history for this message
Jay Vora (Serpent Consulting Services) (jayvora) wrote : | # |
Revision history for this message
Jay Vora (Serpent Consulting Services) (jayvora) wrote : | # |
Interestingly,
The diff of the merge and the commit differ heavily.
http://
I guess its a bug or I am missing something!?!
Thanks.
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | === modified file 'stock/stock.py' | |||
2 | --- stock/stock.py 2011-07-25 09:53:22 +0000 | |||
3 | +++ stock/stock.py 2011-07-27 11:21:46 +0000 | |||
4 | @@ -359,106 +359,118 @@ | |||
5 | 359 | def _product_virtual_get(self, cr, uid, id, product_ids=False, context=None, states=['done']): | 359 | def _product_virtual_get(self, cr, uid, id, product_ids=False, context=None, states=['done']): |
6 | 360 | return self._product_all_get(cr, uid, id, product_ids, context, ['confirmed', 'waiting', 'assigned', 'done']) | 360 | return self._product_all_get(cr, uid, id, product_ids, context, ['confirmed', 'waiting', 'assigned', 'done']) |
7 | 361 | 361 | ||
8 | 362 | def _try_lock_product_reserve(self, cr, uid, location_ids, product_id, product_qty, context=None): | ||
9 | 363 | try: | ||
10 | 364 | # Must lock with a separate select query than the ones used in _product_reserve | ||
11 | 365 | # because FOR UPDATE can't be used with aggregation/group by's | ||
12 | 366 | # (i.e. when individual rows aren't identifiable). | ||
13 | 367 | # We use a SAVEPOINT to be able to rollback this part of the transaction without | ||
14 | 368 | # failing the whole transaction in case the LOCK cannot be acquired. | ||
15 | 369 | cr.execute("SAVEPOINT stock_location_product_reserve") | ||
16 | 370 | # We lock all stock moves in states we are going to consider in the | ||
17 | 371 | # calculation. By locking all DONE move we prevent other transactions | ||
18 | 372 | # from reserving the same products, as they won't be allowed to SELECT | ||
19 | 373 | # them until we're done. | ||
20 | 374 | cr.execute("""SELECT id FROM stock_move | ||
21 | 375 | WHERE product_id=%s | ||
22 | 376 | AND ( | ||
23 | 377 | (location_dest_id IN %s AND state = 'done') | ||
24 | 378 | OR | ||
25 | 379 | (location_id IN %s AND state in ('done', 'assigned')) | ||
26 | 380 | ) | ||
27 | 381 | FOR UPDATE of stock_move NOWAIT""", | ||
28 | 382 | (product_id, location_ids, location_ids), log_exceptions=False) | ||
29 | 383 | except Exception: | ||
30 | 384 | # Here it's likely that the FOR UPDATE NOWAIT failed to get the LOCK, | ||
31 | 385 | # so we ROLLBACK to the SAVEPOINT to restore the transaction to its earlier | ||
32 | 386 | # state, we return False as if the products were not available, and log it: | ||
33 | 387 | cr.execute("ROLLBACK TO stock_location_product_reserve") | ||
34 | 388 | logger = logging.getLogger('stock.location') | ||
35 | 389 | logger.warn("Failed attempt to reserve %s x product %s, likely due to another transaction already in progress. Next attempt is likely to work. Detailed error available at DEBUG level.", product_qty, product_id) | ||
36 | 390 | logger.debug("Trace of the failed product reservation attempt: ", exc_info=True) | ||
37 | 391 | return False | ||
38 | 392 | return True | ||
39 | 393 | |||
40 | 362 | def _product_reserve(self, cr, uid, ids, product_id, product_qty, context=None, lock=False): | 394 | def _product_reserve(self, cr, uid, ids, product_id, product_qty, context=None, lock=False): |
41 | 363 | """ | 395 | """ |
42 | 364 | Attempt to find a quantity ``product_qty`` (in the product's default uom or the uom passed in ``context``) of product ``product_id`` | 396 | Attempt to find a quantity ``product_qty`` (in the product's default uom or the uom passed in ``context``) of product ``product_id`` |
43 | 365 | in locations with id ``ids`` and their child locations. If ``lock`` is True, the stock.move lines | 397 | in locations with id ``ids`` and their child locations. If ``lock`` is True, the stock.move lines |
44 | 366 | of product with id ``product_id`` in the searched location will be write-locked using Postgres's | 398 | of product with id ``product_id`` in the searched location will be write-locked using Postgres's |
46 | 367 | "FOR UPDATE NOWAIT" option until the transaction is committed or rolled back, to prevent reservin | 399 | "FOR UPDATE NOWAIT" option until the transaction is committed or rolled back, to prevent reserving |
47 | 368 | twice the same products. | 400 | twice the same products. |
48 | 369 | If ``lock`` is True and the lock cannot be obtained (because another transaction has locked some of | 401 | If ``lock`` is True and the lock cannot be obtained (because another transaction has locked some of |
49 | 370 | the same stock.move lines), a log line will be output and False will be returned, as if there was | 402 | the same stock.move lines), a log line will be output and False will be returned, as if there was |
50 | 371 | not enough stock. | 403 | not enough stock. |
51 | 372 | 404 | ||
52 | 373 | :param product_id: Id of product to reserve | 405 | :param product_id: Id of product to reserve |
54 | 374 | :param product_qty: Quantity of product to reserve (in the product's default uom or the uom passed in ``context``) | 406 | :param product_qty: Quantity of product to reserve (in the uom passed in ``context``) |
55 | 375 | :param lock: if True, the stock.move lines of product with id ``product_id`` in all locations (and children locations) with ``ids`` will | 407 | :param lock: if True, the stock.move lines of product with id ``product_id`` in all locations (and children locations) with ``ids`` will |
56 | 376 | be write-locked using postgres's "FOR UPDATE NOWAIT" option until the transaction is committed or rolled back. This is | 408 | be write-locked using postgres's "FOR UPDATE NOWAIT" option until the transaction is committed or rolled back. This is |
57 | 377 | to prevent reserving twice the same products. | 409 | to prevent reserving twice the same products. |
63 | 378 | :param context: optional context dictionary: it a 'uom' key is present it will be used instead of the default product uom to | 410 | :param context: context dictionary with 'uom' key mapped to the ID of the UoM to use to compute the product quantities |
64 | 379 | compute the ``product_qty`` and in the return value. | 411 | :return: List of pairs (qty, location_id) with the (partial) quantities that can be taken in each location to |
65 | 380 | :return: List of tuples in the form (qty, location_id) with the (partial) quantities that can be taken in each location to | 412 | reach the requested product_qty (expressed in the requested uom), or False if not enough |
66 | 381 | reach the requested product_qty (``qty`` is expressed in the default uom of the product), of False if enough | 413 | products could be found, or the lock could not be obtained (and ``lock`` was True). |
67 | 382 | products could not be found, or the lock could not be obtained (and ``lock`` was True). | 414 | sum(qty) == ``product_qty``. |
68 | 383 | """ | 415 | """ |
69 | 416 | if context is None: | ||
70 | 417 | context = {} | ||
71 | 418 | location_ids = self.search(cr, uid, [('location_id', 'child_of', ids)]) | ||
72 | 419 | locations_tuple = tuple(location_ids) | ||
73 | 420 | if lock and not self._try_lock_product_reserve(cr, uid, locations_tuple, product_id, product_qty, context=context): | ||
74 | 421 | return False | ||
75 | 422 | |||
76 | 423 | # Giant query to obtain triplets of (product_uom, product_qty, location_id) summing all relevant | ||
77 | 424 | # stock moves quantities per location, with incoming quantities taken positive, | ||
78 | 425 | # and outgoing taken negative. | ||
79 | 426 | cr.execute("""SELECT x.product_uom, SUM(x.coeff * x.product_qty) as product_qty, x.loc_id as location_id | ||
80 | 427 | FROM ( | ||
81 | 428 | SELECT 1.0 as coeff, product_uom, location_dest_id as loc_id, | ||
82 | 429 | sum(product_qty) AS product_qty | ||
83 | 430 | FROM stock_move | ||
84 | 431 | WHERE location_dest_id in %s AND | ||
85 | 432 | location_id != location_dest_id AND | ||
86 | 433 | product_id = %s AND | ||
87 | 434 | state = 'done' | ||
88 | 435 | GROUP BY location_dest_id, product_uom | ||
89 | 436 | UNION | ||
90 | 437 | SELECT -1.0 as coeff, product_uom, location_id as loc_id, | ||
91 | 438 | sum(product_qty) AS product_qty | ||
92 | 439 | FROM stock_move | ||
93 | 440 | WHERE location_id in %s AND | ||
94 | 441 | location_id != location_dest_id AND | ||
95 | 442 | product_id = %s AND | ||
96 | 443 | state in ('done', 'assigned') | ||
97 | 444 | GROUP BY location_id, product_uom | ||
98 | 445 | ) AS x | ||
99 | 446 | GROUP BY x.loc_id, x.product_uom | ||
100 | 447 | """, | ||
101 | 448 | (locations_tuple, product_id, locations_tuple, product_id)) | ||
102 | 449 | sum_rows = cr.fetchall() | ||
103 | 450 | |||
104 | 451 | qty_by_location = {} | ||
105 | 452 | ProductUom = self.pool.get('product.uom') | ||
106 | 453 | target_uom = context.get('uom') | ||
107 | 454 | # Convert all UoMs into target UoM | ||
108 | 455 | for uom_id, qty, loc_id in sum_rows: | ||
109 | 456 | qty_by_location.setdefault(loc_id,0.0) | ||
110 | 457 | qty_by_location[loc_id] += ProductUom._compute_qty(cr, uid, uom_id, qty, target_uom) | ||
111 | 458 | |||
112 | 459 | # to compute final result we handle locations in the | ||
113 | 460 | # order in which they were returned by the original search(). | ||
114 | 384 | result = [] | 461 | result = [] |
192 | 385 | amount = 0.0 | 462 | for loc_id in location_ids: |
193 | 386 | if context is None: | 463 | if loc_id not in qty_by_location: |
194 | 387 | context = {} | 464 | #skip location without this product |
195 | 388 | pool_uom = self.pool.get('product.uom') | 465 | continue |
196 | 389 | for id in self.search(cr, uid, [('location_id', 'child_of', ids)]): | 466 | qty = qty_by_location[loc_id] |
197 | 390 | if lock: | 467 | if qty <= 0.0: |
198 | 391 | try: | 468 | continue |
199 | 392 | # Must lock with a separate select query because FOR UPDATE can't be used with | 469 | qty = min(product_qty, qty) |
200 | 393 | # aggregation/group by's (when individual rows aren't identifiable). | 470 | result.append((qty, loc_id)) |
201 | 394 | # We use a SAVEPOINT to be able to rollback this part of the transaction without | 471 | product_qty -= qty |
202 | 395 | # failing the whole transaction in case the LOCK cannot be acquired. | 472 | if product_qty <= 0.0: |
203 | 396 | cr.execute("SAVEPOINT stock_location_product_reserve") | 473 | return result |
127 | 397 | cr.execute("""SELECT id FROM stock_move | ||
128 | 398 | WHERE product_id=%s AND | ||
129 | 399 | ( | ||
130 | 400 | (location_dest_id=%s AND | ||
131 | 401 | location_id<>%s AND | ||
132 | 402 | state='done') | ||
133 | 403 | OR | ||
134 | 404 | (location_id=%s AND | ||
135 | 405 | location_dest_id<>%s AND | ||
136 | 406 | state in ('done', 'assigned')) | ||
137 | 407 | ) | ||
138 | 408 | FOR UPDATE of stock_move NOWAIT""", (product_id, id, id, id, id), log_exceptions=False) | ||
139 | 409 | except Exception: | ||
140 | 410 | # Here it's likely that the FOR UPDATE NOWAIT failed to get the LOCK, | ||
141 | 411 | # so we ROLLBACK to the SAVEPOINT to restore the transaction to its earlier | ||
142 | 412 | # state, we return False as if the products were not available, and log it: | ||
143 | 413 | cr.execute("ROLLBACK TO stock_location_product_reserve") | ||
144 | 414 | logger = logging.getLogger('stock.location') | ||
145 | 415 | logger.warn("Failed attempt to reserve %s x product %s, likely due to another transaction already in progress. Next attempt is likely to work. Detailed error available at DEBUG level.", product_qty, product_id) | ||
146 | 416 | logger.debug("Trace of the failed product reservation attempt: ", exc_info=True) | ||
147 | 417 | return False | ||
148 | 418 | |||
149 | 419 | # XXX TODO: rewrite this with one single query, possibly even the quantity conversion | ||
150 | 420 | cr.execute("""SELECT product_uom, sum(product_qty) AS product_qty | ||
151 | 421 | FROM stock_move | ||
152 | 422 | WHERE location_dest_id=%s AND | ||
153 | 423 | location_id<>%s AND | ||
154 | 424 | product_id=%s AND | ||
155 | 425 | state='done' | ||
156 | 426 | GROUP BY product_uom | ||
157 | 427 | """, | ||
158 | 428 | (id, id, product_id)) | ||
159 | 429 | results = cr.dictfetchall() | ||
160 | 430 | cr.execute("""SELECT product_uom,-sum(product_qty) AS product_qty | ||
161 | 431 | FROM stock_move | ||
162 | 432 | WHERE location_id=%s AND | ||
163 | 433 | location_dest_id<>%s AND | ||
164 | 434 | product_id=%s AND | ||
165 | 435 | state in ('done', 'assigned') | ||
166 | 436 | GROUP BY product_uom | ||
167 | 437 | """, | ||
168 | 438 | (id, id, product_id)) | ||
169 | 439 | results += cr.dictfetchall() | ||
170 | 440 | total = 0.0 | ||
171 | 441 | results2 = 0.0 | ||
172 | 442 | |||
173 | 443 | for r in results: | ||
174 | 444 | amount = pool_uom._compute_qty(cr, uid, r['product_uom'], r['product_qty'], context.get('uom', False)) | ||
175 | 445 | results2 += amount | ||
176 | 446 | total += amount | ||
177 | 447 | |||
178 | 448 | if total <= 0.0: | ||
179 | 449 | continue | ||
180 | 450 | |||
181 | 451 | amount = results2 | ||
182 | 452 | if amount > 0: | ||
183 | 453 | if amount > min(total, product_qty): | ||
184 | 454 | amount = min(product_qty, total) | ||
185 | 455 | result.append((amount, id)) | ||
186 | 456 | product_qty -= amount | ||
187 | 457 | total -= amount | ||
188 | 458 | if product_qty <= 0.0: | ||
189 | 459 | return result | ||
190 | 460 | if total <= 0.0: | ||
191 | 461 | continue | ||
204 | 462 | return False | 474 | return False |
205 | 463 | 475 | ||
206 | 464 | stock_location() | 476 | stock_location() |
Olivier,
Thanks for proposing merge for one of most awaited bugs.
Sorry to say, but somehow the branch contains some conflicts.
Can you recheck?
Thanks.