Opened 7 years ago

Closed 6 years ago

Last modified 4 years ago

#9206 closed (fixed)

Improve documentation on PostgreSQL transactions, exceptions and savepoints

Reported by: Richard Davies <richard.davies@…> Owned by: jacob
Component: Documentation Version: master
Severity: Keywords:
Cc: richard.davies@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

The documentation on raw SQL queries does not mention the need to call transaction.commit_unless_managed() and transaction.savepoint*() in certain cases. This patch adds to the main documentation on raw SQL, and also replaces a second copy which exists today with a link to the main documentation.

See http://groups.google.com/group/django-developers/browse_thread/thread/620ccff354c859c2 for a heated discussion on Django developers whether the current behavior is good - this patch only seeks to better document where we are today.

Attachments (7)

9206-r9084-raw-sql-docs.diff (4.9 KB) - added by Richard Davies <richard.davies@…> 7 years ago.
Improve documentation on raw SQL queries
9206-r10628-raw-sql-docs.diff (5.9 KB) - added by Richard Davies <richard.davies@…> 6 years ago.
Updated for Malcolm's comments
extra.diff (852 bytes) - added by Richard Davies <richard.davies@…> 6 years ago.
1.1 specific piece missed in r10655
extra_v2.diff (2.6 KB) - added by Richard Davies <richard.davies@…> 6 years ago.
New draft of extra piece
extra_v3.diff (3.8 KB) - added by Richard Davies <richard.davies@…> 6 years ago.
New draft again of extra piece
extra_v4.diff (5.2 KB) - added by Richard Davies <richard.davies@…> 6 years ago.
Another new draft
extra_v5.diff (5.2 KB) - added by Richard Davies <richard.davies@…> 6 years ago.
Fixed typo

Download all attachments as: .zip

Change History (22)

Changed 7 years ago by Richard Davies <richard.davies@…>

Improve documentation on raw SQL queries

comment:1 Changed 7 years ago by mtredinnick

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

Looks good. A few small changes I would suggest to clear up some problems:

  1. Django does not have a mode called "auto-commit". The document says it's similar to a database's auto-commit behaviour, but that means it's not identical and so we shouldn't use that term to describe Django's mode. It will lead to even more confusion than when people don't read the current text. Now, if they do read the text, they'll end up with the wrong term. So let's find a better way of describing that mode (e.g "when Django isn't configured for manual transaction management (via the XYZ setting)" or something similar).
  2. I think this change should also be clear that things like commit_unless_managed() are only needed for data-changing operations. Select queries for example (one of the more common uses of custom SQL) don't require a call to that method and people don't have to go around adding it everywhere just for that case. It currently sounds all very intrusive, but it's really just for one particular style of query, so make that clear.
  3. The documentation of savepoints should go in the transaction document, not in the raw SQL section. It's transaction related, after all. That documentation should be written something along the lines of "if you want your application to work transparently with PostgreSQL 8.x ..." and probably also include a note that savepoints don't work on PostgreSQL 7.x, so if your data changing transaction fails there, you're screwed and have to just roll back.

Changed 6 years ago by Richard Davies <richard.davies@…>

Updated for Malcolm's comments

comment:2 Changed 6 years ago by Richard Davies <richard.davies@…>

  • milestone set to 1.1
  • Version changed from 1.0 to SVN

Here's an update incorporating Malcolm's comments. Note that I de-duplicate descriptions of raw SQL in doc/topics/db/models.txt and doc/topics/db/sql.txt. These are currently marginally different. I'm assuming that sql.txt is the current master version and that the other one is outdated, but the committer should double-check this.

comment:3 Changed 6 years ago by russellm

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

(In [10657]) [1.0.X] Fixed #9206 -- Clarified documentation of transaction handling in raw SQL, and error recovery for Postgres. Thanks to Richard Davies for the suggestion and draft text.

Merge of r10655 from trunk.

Changed 6 years ago by Richard Davies <richard.davies@…>

1.1 specific piece missed in r10655

comment:4 Changed 6 years ago by Richard Davies <richard.davies@…>

  • Resolution fixed deleted
  • Status changed from closed to reopened

Russell - thanks for the check in. One small piece which you missed, so I've attached it as "extra.diff" and reopened the ticket. This last bit is Django 1.1 specific.

comment:5 Changed 6 years ago by russellm

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

Richard: Thanks, but I left that bit out deliberately - I might be missing the point, but I didn't really see what that section was adding to the discussion. The fact that django uses a pseudo-autocommit mode is covered earlier, and isn't Postgres specific. The operation of the database-native autocommit mode is discussed in the docs for the Postgres binding (which is linked to). To me, the 'missing' paragraph only served to confuse the issue of what autocommit meant. This comes back to Malcolm's comment about auto-commit confusion.

I'm fully willing to admit that more documentation may be required - It just isn't obvious to me what is missing (or how the paragraph I omitted could be turned into the missing piece). If you feel there is some vital information that needs to be included, feel free to work up another draft for that paragraph and we'll take a look at it.

Changed 6 years ago by Richard Davies <richard.davies@…>

New draft of extra piece

comment:6 Changed 6 years ago by Richard Davies <richard.davies@…>

  • Resolution fixed deleted
  • Status changed from closed to reopened

OK - I've worked up another draft, which I think explains it better - my point is that savepoints are not necessary for people using the database-native autocommit mode.

comment:7 Changed 6 years ago by russellm

This is better, but two comments:

  1. The versionadded flag doesn't render - you can't have a versionadded in between bullet points like you have used
  1. When discussing savepoints, you present an example. You have tried to keep the example simple, which is good - but as a result, it isn't obvious to me how savepoints are any different to normal rollbacks. This possibly points to a bigger problem - the fact that savepoints aren't documented anywhere (as far as I can make out). If there was a good block of savepoint documentation, you could just use a link to that documentation. If savepoint docs existed, you wouldn't need an example at all - you could just link to the relevant section and move on.

Changed 6 years ago by Richard Davies <richard.davies@…>

New draft again of extra piece

comment:8 Changed 6 years ago by Richard Davies <richard.davies@…>

OK - here's another version which is rather longer and spells out the different behaviours in full, hence answering your comment 2. On reflection, I think all of this detail is worth including, as you say.

On your comment 1, if that use of versionadded does not work then I have no idea how to solve my problem! So I have omitted the flag entirely. The last bullet point is still Django 1.1 specific, so I guess you should either only commit to the Django 1.1 branch, or alternatively also commit to the Django 1.0.x branch with that bullet point removed.

On the issue of separate savepoint documentation, I think you are right that there is none. However, I believe that they were specifically introduced for exactly this situation [8314], so there's no shame in putting the primary documentation here.

comment:9 Changed 6 years ago by Richard Davies <richard.davies@…>

  • Summary changed from Improve documentation on raw SQL queries to Improve documentation on PostgreSQL transactions, exceptions and savepoints

(aside - I believe the behaviour which my last version fully documents is a serious bug in the combination of Django and PostgreSQL. However various tickets to resolve it have gone nowhere (#5043, #9205) so we should at least clearly document it!)

comment:10 Changed 6 years ago by russellm

  • Patch needs improvement set

This draft may be longer, but it isn't any clearer. Look at this from the point of view of someone who doesn't know what a savepoint is - now, what is the difference between a simple transaction rollback, and using a savepoint? In both cases, a.save() and c.save() succeeds, and b.save() fails. The only difference is that if you use savepoints, you need 2 more lines of code. Your example is not sufficiently complex to demonstrate any significant difference between the two use cases.

Again, what is needed here is proper documentation of savepoints, not a bandaid over the problem. It doesn't particularly matter if this is done as part of the transaction docs, or if it is done as a standalone savepoint document. What is important is that the feature - and Django's interface to it - is explained in full.

Also - simply omitting the tag because it doesn't compile isn't an acceptable solution. If something is v1.1 specific, it needs to be marked as such. I will backport these docs with the v1.1 point removed, but that doesn't remove the need to tag the new feature in the v1.1 docs.

Changed 6 years ago by Richard Davies <richard.davies@…>

Another new draft

Changed 6 years ago by Richard Davies <richard.davies@…>

Fixed typo

comment:11 Changed 6 years ago by Richard Davies <richard.davies@…>

  • Patch needs improvement unset

I've split out separate documentation of savepoints, have added some more explanation of the different between savepoint and transaction rollback (the example is sufficiently complex, just wasn't sufficiently well explained) and have reformatted so that I can use versionadded again.

Hope this is now OK!

comment:12 Changed 6 years ago by jacob

  • Owner changed from nobody to jacob
  • Status changed from reopened to assigned

comment:13 Changed 6 years ago by russellm

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

(In [10791]) Fixed #9206 -- Added documentation on savepoints, and how to use them to recover from errors in PostgreSQL. Thanks to Richard Davies for the draft text.

comment:14 Changed 6 years ago by russellm

(In [10792]) [1.0.X] Fixed #9206 -- Added documentation on savepoints, and how to use them to recover from errors in PostgreSQL. Thanks to Richard Davies for the draft text.

Merge of r10791 from trunk.

comment:15 Changed 4 years ago by jacob

  • milestone 1.1 deleted

Milestone 1.1 deleted

Note: See TracTickets for help on using tickets.
Back to Top