database commits everywhere in orm's _auto_init, what purpose ?

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

database commits everywhere in orm's _auto_init, what purpose ?

Valentin LAB
Hi,

I've noticed with great pleasure the introduction of a small savepoint
facility in the "sql_db.py" quite recently, and I looked how transaction
where used in the initialisation / update process.

I don't understand why ``_auto_init`` uses ``cr.commit()`` every now and
them. What is the exact purpose of these ?

Wouldn't it be much better NOT to use them at all ?
And if there are some hidden benefits, what about replacing them with
savepoints ?

My main concern is about having a clean initialisation / update process:
we have issues quite often with modules that are not correctly updated,
and they are marked (and commited) wrongly in unsatisfactory states.
This leaves the whole database often in a intermediary state of which I
can't find any justification. We then face unique issues depending on
which modules failed and from which path we perform multi-update, and
often have to enter the weird multiple-updates process.

I was changing the code to use savepoints to support sort of
"nested-commit", allowing to leave the database untouched if something
went wrong before the full initialisation occured.

But why are these commits there in the first place ? What am I missing ?

Thanks for your enlightenments,

--
Valentin LAB
Ingénieur Développement

tel:  +33 6 71 39 62 13
mail: [hidden email]

_______________________________________________
Mailing list: https://launchpad.net/~openerp-expert-framework
Post to     : [hidden email]
Unsubscribe : https://launchpad.net/~openerp-expert-framework
More help   : https://help.launchpad.net/ListHelp
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: database commits everywhere in orm's _auto_init, what purpose ?

Antony Lesuisse
Yes we would love that too, but refactoring module/*.py is hard and it's not
broken so we dont plan to fix it now. Any MP to cleanup this would be welcome.
Savepoints are not needed at all I would avoid all commits and do everything
in one transaction.

On 03/13/2014 03:16 PM, Valentin LAB wrote:

> Hi,
>
> I've noticed with great pleasure the introduction of a small savepoint
> facility in the "sql_db.py" quite recently, and I looked how transaction where
> used in the initialisation / update process.
>
> I don't understand why ``_auto_init`` uses ``cr.commit()`` every now and them.
> What is the exact purpose of these ?
>
> Wouldn't it be much better NOT to use them at all ?
> And if there are some hidden benefits, what about replacing them with
> savepoints ?
>
> My main concern is about having a clean initialisation / update process: we
> have issues quite often with modules that are not correctly updated, and they
> are marked (and commited) wrongly in unsatisfactory states. This leaves the
> whole database often in a intermediary state of which I can't find any
> justification. We then face unique issues depending on which modules failed
> and from which path we perform multi-update, and often have to enter the weird
> multiple-updates process.
>
> I was changing the code to use savepoints to support sort of "nested-commit",
> allowing to leave the database untouched if something went wrong before the
> full initialisation occured.
>
> But why are these commits there in the first place ? What am I missing ?
>
> Thanks for your enlightenments,
>

_______________________________________________
Mailing list: https://launchpad.net/~openerp-expert-framework
Post to     : [hidden email]
Unsubscribe : https://launchpad.net/~openerp-expert-framework
More help   : https://help.launchpad.net/ListHelp
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: database commits everywhere in orm's _auto_init, what purpose ?

Valentin LAB
In reply to this post by Valentin LAB
Here's a MP to propose a simple way to add transaction where it could be
needed without breaking legacy code:

https://code.launchpad.net/~0k.io/openobject-server/trunk-new-prevent-cr-commit-to-allow-super-transaction/+merge/210884

If this is okay, it's trivial to encompass openerp's initialize / update
sequence in one go.

Any comments ?


On 13/03/14 15:16, Valentin LAB wrote:

> Hi,
>
> I've noticed with great pleasure the introduction of a small savepoint
> facility in the "sql_db.py" quite recently, and I looked how transaction
> where used in the initialisation / update process.
>
> I don't understand why ``_auto_init`` uses ``cr.commit()`` every now and
> them. What is the exact purpose of these ?
>
> Wouldn't it be much better NOT to use them at all ?
> And if there are some hidden benefits, what about replacing them with
> savepoints ?
>
> My main concern is about having a clean initialisation / update process:
> we have issues quite often with modules that are not correctly updated,
> and they are marked (and commited) wrongly in unsatisfactory states.
> This leaves the whole database often in a intermediary state of which I
> can't find any justification. We then face unique issues depending on
> which modules failed and from which path we perform multi-update, and
> often have to enter the weird multiple-updates process.
>
> I was changing the code to use savepoints to support sort of
> "nested-commit", allowing to leave the database untouched if something
> went wrong before the full initialisation occured.
>
> But why are these commits there in the first place ? What am I missing ?
>
> Thanks for your enlightenments,
>


--
Valentin LAB
Ingénieur Développement

tel:  +33 6 71 39 62 13
mail: [hidden email]

_______________________________________________
Mailing list: https://launchpad.net/~openerp-expert-framework
Post to     : [hidden email]
Unsubscribe : https://launchpad.net/~openerp-expert-framework
More help   : https://help.launchpad.net/ListHelp
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: database commits everywhere in orm's _auto_init, what purpose ?

Antony Lesuisse
The api looks ok, even if i think we should discourage the use of savepoints.
@chs what do you think?

On 03/13/2014 08:05 PM, Valentin LAB wrote:

> Here's a MP to propose a simple way to add transaction where it could be
> needed without breaking legacy code:
>
> https://code.launchpad.net/~0k.io/openobject-server/trunk-new-prevent-cr-commit-to-allow-super-transaction/+merge/210884
>
>
> If this is okay, it's trivial to encompass openerp's initialize / update
> sequence in one go.
>
> Any comments ?
>
>
> On 13/03/14 15:16, Valentin LAB wrote:
>> Hi,
>>
>> I've noticed with great pleasure the introduction of a small savepoint
>> facility in the "sql_db.py" quite recently, and I looked how transaction
>> where used in the initialisation / update process.
>>
>> I don't understand why ``_auto_init`` uses ``cr.commit()`` every now and
>> them. What is the exact purpose of these ?
>>
>> Wouldn't it be much better NOT to use them at all ?
>> And if there are some hidden benefits, what about replacing them with
>> savepoints ?
>>
>> My main concern is about having a clean initialisation / update process:
>> we have issues quite often with modules that are not correctly updated,
>> and they are marked (and commited) wrongly in unsatisfactory states.
>> This leaves the whole database often in a intermediary state of which I
>> can't find any justification. We then face unique issues depending on
>> which modules failed and from which path we perform multi-update, and
>> often have to enter the weird multiple-updates process.
>>
>> I was changing the code to use savepoints to support sort of
>> "nested-commit", allowing to leave the database untouched if something
>> went wrong before the full initialisation occured.
>>
>> But why are these commits there in the first place ? What am I missing ?
>>
>> Thanks for your enlightenments,
>>
>
>

_______________________________________________
Mailing list: https://launchpad.net/~openerp-expert-framework
Post to     : [hidden email]
Unsubscribe : https://launchpad.net/~openerp-expert-framework
More help   : https://help.launchpad.net/ListHelp
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: database commits everywhere in orm's _auto_init, what purpose ?

Olivier Dony (OpenERP)
> On 03/13/2014 08:05 PM, Valentin LAB wrote:
>> Here's a MP to propose a simple way to add transaction where it could be
>> needed without breaking legacy code:
>>
>> https://code.launchpad.net/~0k.io/openobject-server/trunk-new-prevent-cr-commit-to-allow-super-transaction/+merge/210884
>>
>>
>> If this is okay, it's trivial to encompass openerp's initialize / update
>> sequence in one go.
>>
>> Any comments ?


I find this change quite dangerous. Any magic that affects all running
transactions is particularly likely to introduce subtle bugs that we will only
notice long after merging it. The rowcount one is just one example of
not-so-obvious regressions that will happen.

As you've seen, calling legacy code is simply not safe in the context of
savepoints (even after your patch), so we should just be careful with the use
of cr.savepoint() and avoid extra magic.
The point of the cr.savepoint() contextmanager is to automatically close your
savepoint, not to be a safeguard for legacy code.

In addition, I think the API should preserve programmers from surprises as much
as possible. Anyone using commit() explicitly is either:
  1. A clueless programmer who should not be using them, and will manage to
screw it, regardless of the API we choose ;
  2. Someone who actually knows what SAVEPOINT, COMMIT, ROLLBACK and ROLLBACK
TO SAVEPOINT mean, and will expect cr.commit() and cr.rollback() to do what
they say at all times.

Regarding the issue of commits in auto_init(), I had tried in the past to get
rid of them (they have no reason to be there indeed), but never merged that
patch because it was causing obscure errors that I had no time to investigate.
I would be glad if anyone had the time to look at it.

_______________________________________________
Mailing list: https://launchpad.net/~openerp-expert-framework
Post to     : [hidden email]
Unsubscribe : https://launchpad.net/~openerp-expert-framework
More help   : https://help.launchpad.net/ListHelp
Loading...