[ADD] speedup sale migration by prepopulating computed fields by hbrunn · Pull Request #710 · OCA/OpenUpgrade

hbrunn · 2016-12-26T16:24:12Z

Description of the issue/feature this PR addresses:

Current behavior before PR:

Desired behavior after PR is merged:

--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

pedrobaeza · 2016-12-26T16:32:14Z

Shouldn't this be ids[x[0] for x in ...]?

no, look closely for the comma

Ah, a bit weird syntax, but OK, you're right

the weird syntax is actually slightly faster - compare

python -m timeit '[x for x, _ in [(y, y) for y in range(100000)]]'

and

python -m timeit '[x[0] for x in [(y, y) for y in range(100000)]]'

Noted for future list comprehension that I make.

this can also be helpful in a normal loop when you iterate over a huge result set, but do quite little per iteration. Then

for a, b, c, d in cr.fetchall()

performs better than

for row in cr.fetchall()

if within the loop, all values of row are used

Thanks for the info. The problem is if the list if very huge or you don't know all possible values (in a select * statement), but obviously that cases can be always contained.

[FIX] we did it the wrong way around

NL66278 · 2016-12-29T12:59:46Z

It might speed things up even more if you first add all the fields to a table and then fill the values with one statement. Now you have to go several times to all or most of the sale orders and lines.

hbrunn · 2016-12-29T13:07:33Z

yes, good point.

But at the moment I'm busy with a general speedup of recomputations of which I promise myself much more performance gains, because something is fishy here. It runs currently for testing purposes, afterwards I'll propose this to upstream, OCB and here hbrunn/OCB@25e5f95

There are two problems with current recomputation: The time used grows nonlinear, that's a problem with my 300k sale order lines. Additionally, what makes this hard to debug is that there's some nondeterminism going on, two times recomputing the same field on the same rows yields a different execution path (or the same, that's up to chance).

NL66278 · 2017-01-13T12:42:55Z

Apart from the general rewrite of recomputation you might also look at this PR: #735
Allows to just stop the expensive recomputations, while having Odoo create the fields themselves. Consequence is that the actual data_migration must move to the post_migration

JordiBForgeFlow · 2017-03-27T23:33:37Z

+    cr.execute(
+        """\
+        update sale_order_line
+        set qty_to_invoice=product_uom_qty - qty_invoiced


@hbrunn This sql sentence is not correct. It does not replicate the same conditions as https://github.com/odoo/odoo/blob/9.0/addons/sale/sale.py#L516

For example, in products where the invoice policy is set based on delivered quantities the quantity to invoice is to be based on the quantity delivered - quantity invoiced.

JordiBForgeFlow · 2017-03-28T00:31:37Z

+        """\
+        update sale_order_line
+        set invoice_status=(
+            case when qty_to_invoice<=0 then 'invoiced'


replace with case when coalesce(qty_to_invoice,0)<=0 then 'invoiced'

JordiBForgeFlow · 2017-03-29T14:25:27Z

+        """\
+        update sale_order_line
+        set qty_to_invoice=product_uom_qty - qty_invoiced
+        where qty_invoiced is not null


This where condition is not correct. Instead use:

update sale_order_line set qty_to_invoice=product_uom_qty - coalesce(qty_invoiced,0)

This issue has been giving me a hard time.

hbrunn · 2017-04-04T16:06:33Z

@jbeficent thanks for you input, done. Next time, just create a PR

pedrobaeza · 2017-05-08T08:20:36Z

What is the status of this?

hbrunn · 2017-05-08T09:25:01Z

I'm waiting for @jbeficent to test the last commit where I integrated his points

pedrobaeza · 2017-05-16T13:35:03Z

@jbeficent can you review? Travis is red anyway.

omar7r · 2017-06-20T16:06:19Z

+        """\
+        update sale_order_line
+        set qty_to_invoice=case
+            when pt.invoice_policy = 'order' then


invoice_policy field in product.template doesn't exists in pre-migration.py because it's a new field in 9.0 version If you move this query to post-migrations it works

pedrobaeza · 2017-08-07T14:16:31Z

@jbeficent please feedback about this PR.

@hbrunn check last comment and red status

hbrunn · 2017-08-08T09:02:02Z

@pedrobaeza last comment defeats the purpose of the pr, the code is fixed but travis is acting up

pedrobaeza · 2017-08-08T10:41:13Z

Again the TRAVIS_COMMIT error, so I close and reopen the PR for forcing a correct build.

pedrobaeza · 2017-08-08T11:17:12Z

+def prepopulate_fields(cr):
+    """ Recomputing a fields will be very expensive via the ORM, so do it
+    here for fields where the computation is trivial"""
+    cr.execute('alter table sale_order_line add column qty_invoiced numeric')


ADD set default=0

pedrobaeza · 2017-08-08T11:18:27Z

+        set qty_invoiced=line2qty_invoiced.qty_invoiced
+        from line2qty_invoiced where line2qty_invoiced.id=sale_order_line.id
+        """)
+    cr.execute('alter table sale_order_line add column qty_to_invoice numeric')


ADD set default=product_uom_qty

About this PG says: cannot use column references in default expression

From PG doc:

DEFAULT default_expr

The DEFAULT clause assigns a default data value for the column whose column definition it appears within. The value is any variable-free expression (subqueries and cross-references to other columns in the current table are not allowed). The data type of the default expression must match the data type of the column.

The default expression will be used in any insert operation that does not specify a value for the column. If there is no default for a column, then the default is null.

OK, I didn't know. Then we should still keep here 2 sentences

I'm working on hbrunn#5. I hope next week this can be merged.

pedrobaeza · 2017-08-08T11:19:10Z

+        where l.order_id=o.id
+        """)
+    cr.execute('alter table sale_order_line add column invoice_status varchar')
+    cr.execute("update sale_order_line set invoice_status='no'")


Instead of this, just add in the previous statement set default='no'

pedrobaeza · 2017-08-08T11:19:40Z

+        where state in ('sale', 'done')
+        """)
+    cr.execute('alter table sale_order add column invoice_status varchar')
+    cr.execute("update sale_order set invoice_status='no'")


The same (set default='no')

mvaled · 2017-09-27T21:25:20Z

Should we merge this?

mvaled · 2017-09-27T21:27:20Z

I'm testing this today. I'll be reporting later (tomorrow perhaps)

mvaled · 2017-09-27T23:26:39Z

TL;DR It seems this is incorrect.

Experiment

I took a DB with Odoo 8, and clone it twice:

createdb -T o8 o9base
createdb -T o8 o9speedy

Then I migrated the o9base without this patch (https://github.com/merchise-autrement/openupgrade/tree/merchise-develop-9.0) and the o9speedy with the patch (https://github.com/merchise-autrement/OpenUpgrade/tree/merchise-9.0-sale-speedup).

However comparing the resulting sale_order_line and sale_order gives a lot of differences:

For instance, the field sale.order.line.qty_invoiced is different in almost all lines:

$ diff -u <(psql -d o9speedy -Ac 'select id, qty_invoiced from sale_order_line where order by id;') <(psql -d o9base -Ac 'select id, qty_invoiced from sale_order_line order by id;') | wc -l
191365

The first thing I notice is that where the o9base get 0, o9speedy gets NULL. Maybe this is not a problem. Because we can always do UPDATE sale_order_line SET qty_invoiced=0 WHERE qty_invoiced IS NULL. Let's do it:

$ psql -d o9speedy -c "UPDATE sale_order_line SET qty_invoiced=0 WHERE qty_invoiced IS NULL"
UPDATE 62646

However, we still have a lot of differences:

$ diff -u <(psql -d o9speedy -Ac 'select id, qty_invoiced from sale_order_line order by id;') <(psql -d o9base -Ac 'select id, qty_invoiced from sale_order_line order by id;') | wc -l
82575

Let's see some records that differ:

-38|15.0000000000000000
-39|1.00000000000000000000
+38|0
+39|0

For the record 38, the speedier version computes 15 while the normal version computes 0. Other fields show as much differences as well. I'll try to look at these differences and see if I can spot the issue.

The Python implementation tests for the invoice's type being other than 'cancel'.

mvaled · 2017-09-28T13:13:26Z

I pushed 8625c10 to fix the qty_invoiced amount.

hbrunn · 2017-09-28T13:15:54Z

can you PR this to this PR's branch? Then I'll merge this in this PR. If you want to speed up getting this merged, you can check @pedrobaeza's comments to. I won't have time to address them any time soon.

mvaled · 2017-09-28T13:20:12Z

I already did: hbrunn#4

[FIX] SQL computation for sale.order.line.qty_invoice.

mvaled · 2017-09-28T13:22:31Z

I'll be addressing @pedrobaeza's comments soon.

mvaled · 2017-09-28T17:27:41Z

See PR hbrunn#5

mvaled · 2017-09-29T14:22:45Z

I created another PR to the original branch. It addresses @pedrobaeza's requests. One of the requested defaults cannot be set by SQL.

mvaled · 2017-09-29T15:15:15Z

Still working on this. The field qty_to_invoice shows differences.

pedrobaeza · 2017-10-22T13:07:22Z

Any news on this?

mvaled · 2017-10-23T13:47:16Z

I've been unable to keep working on this for the past weeks. We're heavily busy migrating ~50 addons to Odoo 9 and then to Odoo 10. After they all migrated to 9, I could try another round of DB migration and see if I could work on this.

pedrobaeza · 2017-10-23T17:03:32Z

OK, let me know. I don't see this as critical because times are more or less manageable in my cases, but maybe with a lot of sale orders it's not the same.

pedrobaeza · 2018-06-23T18:02:44Z

Closing this due to errors on the migration. If anyone wants to continue this experiment, can take this code and go on with the needed changes, proposing the new alternative.

hbrunn added the needs review label Dec 26, 2016

hbrunn added this to the 9.0 milestone Dec 26, 2016

pedrobaeza reviewed Dec 26, 2016

View reviewed changes

hbrunn force-pushed the 9.0-sale_speedup branch from 49ed420 to b4a6a78 Compare December 26, 2016 17:24

[ADD] speedup sale migration by prepopulating computed fields

19e16d8

hbrunn force-pushed the 9.0-sale_speedup branch from 8a9bb87 to 19e16d8 Compare December 26, 2016 18:46

hbrunn added work in progress and removed needs review labels Dec 28, 2016

[FIX] it's not so hard to do the quantity conversions too

e5a49b4

[FIX] we did it the wrong way around

hbrunn force-pushed the 9.0-sale_speedup branch from 88b2082 to e5a49b4 Compare December 28, 2016 14:14

hbrunn mentioned this pull request Jan 10, 2017

[8.0] stock migration very slow #724

Closed

JordiBForgeFlow reviewed Mar 27, 2017

View reviewed changes

JordiBForgeFlow reviewed Mar 28, 2017

View reviewed changes

JordiBForgeFlow reviewed Mar 29, 2017

View reviewed changes

[FIX] community review

b52a820

omar7r reviewed Jun 20, 2017

View reviewed changes

[FIX] don't try to cover impossible case

bb1e349

pedrobaeza closed this Aug 8, 2017

pedrobaeza reopened this Aug 8, 2017

pedrobaeza requested changes Aug 8, 2017

View reviewed changes

[FIX] SQL computation for sale.order.line.qty_invoice.

8625c10

The Python implementation tests for the invoice's type being other than 'cancel'.

Merge pull request #4 from merchise-autrement/hbrunn-9.0-sale_speedup

8c4e68f

[FIX] SQL computation for sale.order.line.qty_invoice.

mvaled mentioned this pull request Sep 28, 2017

WIP: Correct SQL computations. hbrunn/OpenUpgrade#5

Closed

pedrobaeza closed this Jun 23, 2018

Uh oh!

Conversation

hbrunn commented Dec 26, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hbrunn Dec 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NL66278 commented Dec 29, 2016

Uh oh!

hbrunn commented Dec 29, 2016

Uh oh!

NL66278 commented Jan 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hbrunn commented Apr 4, 2017

Uh oh!

pedrobaeza commented May 8, 2017

Uh oh!

hbrunn commented May 8, 2017

Uh oh!

pedrobaeza commented May 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pedrobaeza commented Aug 7, 2017

Uh oh!

hbrunn commented Aug 8, 2017

Uh oh!

pedrobaeza commented Aug 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mvaled Sep 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mvaled Sep 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mvaled commented Sep 27, 2017

Uh oh!

mvaled commented Sep 27, 2017

Uh oh!

mvaled commented Sep 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Experiment

Uh oh!

mvaled commented Sep 28, 2017

Uh oh!

hbrunn commented Sep 28, 2017

Uh oh!

hbrunn Dec 26, 2016 •

edited

Loading

mvaled Sep 29, 2017 •

edited

Loading

mvaled Sep 29, 2017 •

edited

Loading

mvaled commented Sep 27, 2017 •

edited

Loading