Code

#19442 closed Cleanup/optimization (invalid)

Documentation: db transactions clarification

Reported by: startup.canada@… Owned by: nobody
Component: Documentation Version: 1.4
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

In section https://docs.djangoproject.com/en/dev/topics/db/transactions/#requirements-for-transaction-handling says "Django requires that every transaction that is opened is closed before the completion of a request. If you are using autocommit() (the default commit mode) or commit_on_success(), this will be done for you automatically.", giving the impression that given the autocommit we never need to perform commits, but this is not correct for the case of writing raw sql inserts, as explained in https://docs.djangoproject.com/en/dev/topics/db/sql/#executing-custom-sql-directly "After performing a data changing operation, you should then call transaction.commit_unless_managed() to ensure your changes are committed to the database".

Just for clarification I suggest adding in the #requirements-for-transaction-handling a sentence mentioning the need for manually committing for raw sql inserts and/or linking to the executing-custom-sql-directly section.

Attachments (2)

transaction_behaviour.diff (2.8 KB) - added by akaariai 19 months ago.
19442.diff (2.5 KB) - added by timo 19 months ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 19 months ago by aaugustin

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 19 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from new to closed

In 31f49f1396c4cc565017066e9f17c6b850a89687:

Fixed #19442 - Clarified that raw SQL must be committed.

Thanks startup.canada for the suggestion.

comment:3 Changed 19 months ago by Tim Graham <timograham@…>

In e11bd5c5ac0c7d85723451f8da7b6ad30fefc680:

[1.5.X] Fixed #19442 - Clarified that raw SQL must be committed.

Thanks startup.canada for the suggestion.

Backport of 31f49f1396 from master

comment:4 Changed 19 months ago by akaariai

The modified docs suggest that both autocommit and commit_on_success need manual commit for raw SQL. I don't think this is the case - if you are using raw SQL, then commit_on_success should just work, but autocommit fails to commit your raw SQL writes (as it doesn't know which raw SQL statements are writes...).

comment:5 Changed 19 months ago by timo

  • Resolution fixed deleted
  • Status changed from closed to reopened

I guess the linked section (/topics/db/sql/#executing-custom-sql-directly) should be clarified as well as it seems to suggest you need to call transaction.commit_unless_managed regardless of transaction mode.

comment:6 Changed 19 months ago by akaariai

It is possible that I just remember this wrong, though I am pretty sure the behaviour is as I said in comment:4. I will try to double-check how the decorators work later today. It is also possible that the behaviour has changed along the years and not all parts of the docs reflect this.

Changed 19 months ago by akaariai

comment:7 Changed 19 months ago by akaariai

The added test case (not meant to be included in the test suite) shows how the autocommit and commit_on_success work. Basically, autocommit is somewhat dangerous, and somewhat easily leads to transaction leak.

Anyhow, the point for this ticket is that no, you don't need to commit raw SQL when using commit_on_success.

I do think it would be a good idea to rewrite Django's transaction handling. It currently works nicely in request processing, somewhat works in single db shell scripts and is really hard to use in multidb shell scripts. But this of course isn't this ticket's problem.

Changed 19 months ago by timo

comment:8 Changed 19 months ago by timo

  • Has patch set

Thanks for your research! Updated patch attached. Seems that Aymeric doesn't like the idea of rewriting transaction management for offline tasks (see #13870). I'd be appreciative if you'd like to review the documentation patch on that ticket as well.

comment:9 Changed 19 months ago by akaariai

Maybe rewrite of transaction management is a little drastic, but adding some more helpers at least would make sense.

We would likely want to add docs of how to use transactions safely in Celery jobs (or other long running processes which aren't serving requests). Here the basic idea is that if you make sure _all_ connections are closed after a job is ran, then you can proceed like in request processing. If you don't close all connections, then you must be extra careful in making sure no transactions are left open. #13870 is of course the right ticket for work on this area.

I will take a closer look at the whole transaction management documentation. The patch is definitely an improvement to current docs, but I wonder if there is something more lurking in there...

comment:10 Changed 18 months ago by timo

Shall we commit this patch, mark it as needs improvement, and/or open a ticket for following up with additional doc work per your comment?

comment:11 Changed 16 months ago by aaugustin

  • Status changed from reopened to new

comment:12 Changed 16 months ago by timo

  • Resolution set to invalid
  • Status changed from new to closed

Don't think this is relevant anymore with the merge of the database-level-autocommit branch [14cddf51c5].

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.