Re: Request for review: database-level autocommit

2013-03-09 Thread Aymeric Augustin 
On 9 mars 2013, at 00:51, Florian Apolloner  wrote:

> If you don't care about the timeframe I'd like to smoke test it on 
> Postgresql,  Mysql, Oracle on Tuesday (again -- due to an exam I won't have 
> time for earlier tests [although I am pretty sure postgres and mysql are in a 
> good shape after my previous tests]). In the worst case scenario I'd ask you 
> to give me till the 16th.

I'd like to merge it before the PyCon sprints to get some feedback and to avoid 
blocking development on the ORM.

> I'd also like to ask if we really want to have savepoints as default.

savepoint=False is primarily designed for internal use in Django. I hesitated 
before making it a public API; finally I documented it because I thought people 
would discover it anyway.

Anssi made a benchmark with 1000 raw SQL updates: 
https://code.djangoproject.com/ticket/2227#comment:34 Adding a savepoint around 
each query made it run 2.5x slower. (I don't know if he was updating the same 
object; if he was, that's very pathological, because it can all be done in 
memory without savepoints, while it requires syncing every request to disk with 
savepoints.) Anyway, without savepoint=False, Django could become slower on 
large series of operations that require being in a transaction, like 
multi-table saves.

Now, in your own code, if you're looping on 1000 operations, and you feel 
concerned about SQL performance, I expect you to write this:

with transaction.atomic():  # do all the inserts in a transaction 
and commit only once (faster)
for i in range(1000):
MyModel.objects.create(…)

Not this:

for i in range(1000):
with transaction.atomic():  # WTF?
MyModel.objects.create(…)

The point of savepoint=False is to prevent Django from implicitly doing the 
latter. It's only useful for constructs where the overhead of the savepoint 
becomes noticeable, ie. fast queries that are likely to be executed in a loop.

> Looking at my pluggable projects I'll usually have @atomic(savepoints=False) 
> around __every__ complex view since I don't know if ATOMIC_REQUESTS will be 
> true or false. Even if I use it as context manager most views won't care 
> about transactions vs savepoints, so in most cases ensuring a transaction at 
> all should be enough.


In non-trivial cases, the overhead of the savepoint will not even be 
measurable, and it provides more natural error handling. I've forgotten to 
document it; I'll do it now.

The savepoint guarantees that you can always go ahead after:

try:
with transaction.atomic():
do_stuff()
except ...:
handle_exception()

See also 
https://docs.djangoproject.com/en/dev/topics/db/transactions/#savepoint-rollback

-- 
Aymeric.


-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers?hl=en.
For more options, visit https://groups.google.com/groups/opt_out.

Re: Request for review: database-level autocommit

2013-03-08 Thread Florian Apolloner 
Hi Aymeric,

On Friday, March 8, 2013 11:32:52 PM UTC+1, Aymeric Augustin wrote:
>
> Carl was kind enough to review the branch in detail. (Thank you!)
>
> I'll take his feedback into account, clean up the history, and push a new
> version tomorrow. I hope to merge it during the week-end.
>

If you don't care about the timeframe I'd like to smoke test it on 
Postgresql,  Mysql, Oracle on Tuesday (again -- due to an exam I won't have 
time for earlier tests [although I am pretty sure postgres and mysql are in 
a good shape after my previous tests]). In the worst case scenario I'd ask 
you to give me till the 16th.

I'd also like to ask if we really want to have savepoints as default. 
Looking at my pluggable projects I'll usually have 
@atomic(savepoints=False) around __every__ complex view since I don't know 
if ATOMIC_REQUESTS will be true or false. Even if I use it as context 
manager most views won't care about transactions vs savepoints, so in most 
cases ensuring a transaction at all should be enough.

Regards,
Florian

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers?hl=en.
For more options, visit https://groups.google.com/groups/opt_out.

Re: Request for review: database-level autocommit

2013-03-08 Thread Aymeric Augustin 
On 7 mars 2013, at 17:37, Aymeric Augustin  
wrote:

> Barring unexpected problems and last minute vetoes, I plan to merge
> this branch when another core dev reviews it thoroughly and accepts it,


Carl was kind enough to review the branch in detail. (Thank you!)

I'll take his feedback into account, clean up the history, and push a new
version tomorrow. I hope to merge it during the week-end.

-- 
Aymeric.



-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers?hl=en.
For more options, visit https://groups.google.com/groups/opt_out.

Re: Request for review: database-level autocommit

2013-03-07 Thread Aymeric Augustin 
On 7 mars 2013, at 17:37, Aymeric Augustin  
wrote:

> My work on transaction management is now ready for review:
> https://github.com/django/django/pull/873


It also fixes at least ten tickets. Here's a short explanation for those I 
found.

#2227: `atomic` supports nesting.
#6623: `commit_manually` is deprecated and `atomic` doesn't suffer from this 
defect.
#8320: the problem wasn't identified, but the legacy transaction management is 
deprecated.
#10744: the problem wasn't identified, but the legacy transaction management is 
deprecated.
#10813: since autocommit is enabled, it isn't necessary to rollback after 
errors any more.
#13742: savepoints are now implemented for SQLite.
#13870: transaction management in long running processes isn't a problem any 
more, and it's documented.
#14970: while it digresses on transaction management, this ticket essentially 
asks for autocommit on PostgreSQL.
#15694: `atomic` supports nesting.
#17887: autocommit makes it impossible for a connection to stay "idle of 
transaction".

-- 
Aymeric.

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers?hl=en.
For more options, visit https://groups.google.com/groups/opt_out.

Request for review: database-level autocommit

2013-03-07 Thread Aymeric Augustin 
My work on transaction management is now ready for review:
https://github.com/django/django/pull/873


To sum up, the main improvements over the current implementation are:

1) A better API for transactions
- Simple: just one function covers most of the use cases
- Correct: it actually guarantees atomicity
- Pythonic: it's fully nestable

2) No implicit transactions
- More predictable
- Better performance
- Eliminates dangling transactions ("idle in transaction")
- even in long running processes

3) More maintainable
- More documentation
- Less code, at least from 1.8 onwards
- Closer to the database — avoids problems with the adapters

The main concern is backwards-compatibility. It was discussed at
length in the previous thread. I acknowledge the consequences,
and I documented them.


Here's what you should know before starting a review.

I've rewritten history heavily to make the branch as reviewable as
possible.

I refactored away the old transaction management code in small steps
to avoid introducing unintentional backwards-incompatibilities. I suggest
reviewing the code changeset by changeset.

Some commits make very large edits to the docs and are impossible to
review. I recommend reading only the final version of the docs.


I will avoid force-pushing from now on. Please checkout my branch and
test it!

Barring unexpected problems and last minute vetoes, I plan to merge
this branch when another core dev reviews it thoroughly and accepts it,
or before the PyCon sprints (in 10 days), whichever comes first. Let me
know if you'd like more time to review it.

-- 
Aymeric.

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers?hl=en.
For more options, visit https://groups.google.com/groups/opt_out.