Avoid rebulding mptt tree multiple times in a database update

Currently, when a the database its updated (with --all) the mptt tree is rebuild for every module that registers a model. This is unnecessary, as it's only needed to rebuild it one time per model.

I will upload a review that implements it.

assigned to @pokoli

added trytond type::performance + 1 deleted label

The attached review implements it

New changeset ac3c495d797f by Sergi Almacellas Abellana in branch 'default':
Only rebuild mptt tree one time per model in a database update
http://hg.tryton.org/trytond/rev/ac3c495d797f

added 1 deleted label and removed 1 deleted label

closed

I don't agree with this change.

added 1 deleted label and removed 1 deleted label

reopened

El 28/12/15 a les 14:19, Cédric Krier ha escrit:
> I don't agree with this change.
Why not?

There are many other cases where the rebuild should be trigger from an other module. For example, if there is a change in the left/right name.

Also the test is wrong for example left/right could be added by a first module and the Many2One by a second.

El 28/12/15 a les 14:21, Cédric Krier ha escrit:
> There are many other cases where the rebuild should be trigger from an other module. For example, if there is a change in the left/right name.

Don't see your point: Why a module should change the left or right name?

I will explain a little bit our current scenario.

We have a setup with 31 companies and 2800 accounts per company, which
means 88000 records in the account table. This makes the rebuild process
spend 2 minutes on every run.

Also we have 11 installed modules that override some property of the
account.account module, so this means that trytond is spending 22
minutes (2 minutes for every module) rebuilding the tree of a module
which have already been rebuilded before.

El 28/12/15 a les 14:31, Cédric Krier ha escrit:
> Also the test is wrong for example left/right could be added by a first module and the Many2One by a second.
Don't see the problem, AFAIU with the patch the tree will be rebuilded
correctly, as (Correct me if I'm wrong), the search is not searching for
both fields registered by the module. Only one of them.

I don't care you spend 2 mins to update as far as it does what it must.
Your patch breaks the way it worked.
You prevent to do something like my examples that worked before.
Now we have in trunk a code that doesn't work correctly any more for the cases I gave. I don't care about the "why" such code could exist.

So now we have a bug instead of a feature.

added type::behavior label and removed type::performance label

Other case, the module of ir.model.field could be wrong if for example a field was defined in one module then remove, and another one with the same name is added via another module.

For me, the code should be simply reverted.

assigned to @ced and unassigned @pokoli

BTW, if the ir.model.field is wrong, this should be reported in a separate bug, shouldn't it?

No where it is stated that the module value of ir.model.field will always be <I don't even know what to put here>. The only usage is for translation which should be performed on a clean database.

PS: Don't assign me bug, please fix yours yourself.

assigned to @pokoli and unassigned @ced

So I backed out the current solution, and lets try a fix for all the cases.

added type::performance label and removed type::behavior label

Ok, so for me the issue is more due to the current design of this rebuild tree.
Most of the time, it doesn't need to rebuild the tree because nothing changed during the register process.
So we need a quick way to detect that the tree is wrong. Maybe if there are left/right with 0 as value.

I think that the best solution is to rebuild the trees at the end of the update process, like done with the _history tables [1], in this case we can store all the models that where registered and rebuild the tree only for the ones that have a tree model fields.

What do you think?

[1] http://hg.tryton.org/trytond/file/ac3c495d797f/trytond/modules/__init__.py#l282

On 2015-12-28 16:12, Sergi Almacellas Abellana wrote:
> I think that the best solution is to rebuild the trees at the end of the update process, like done with the _history tables [1], in this case we can store all the models that where registered and rebuild the tree only for the ones that have a tree model fields.
>
> What do you think?

No because some migration process could rely on the MPTT to be up to
date.

El 28/12/15 a les 16:11, Cédric Krier ha escrit:
> Ok, so for me the issue is more due to the current design of this rebuild tree.
> Most of the time, it doesn't need to rebuild the tree because nothing changed during the register process.
> So we need a quick way to detect that the tree is wrong. Maybe if there are left/right with 0 as value.

Makes sense to me, but i will go for the default value of the field (or
None).

I will try to make a patch, as it not seems so complex.

So I uploaded review16921002 which implements the proposed change

added 1 deleted label and removed 1 deleted label

New changeset b81abbbdb1e7 by Sergi Almacellas Abellana in branch 'default':
Only rebuild mptt tree if needed
http://hg.tryton.org/trytond/rev/b81abbbdb1e7

added 1 deleted label and removed 1 deleted label

closed