#9206 closed (fixed)
Improve documentation on PostgreSQL transactions, exceptions and savepoints
Reported by: | Owned by: | Jacob | |
---|---|---|---|
Component: | Documentation | Version: | dev |
Severity: | Keywords: | ||
Cc: | richard.davies@… | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
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)
Change History (22)
by , 16 years ago
Attachment: | 9206-r9084-raw-sql-docs.diff added |
---|
comment:1 by , 16 years ago
Triage Stage: | Unreviewed → Accepted |
---|
Looks good. A few small changes I would suggest to clear up some problems:
- 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).
- 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. - 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.
comment:2 by , 16 years ago
milestone: | → 1.1 |
---|---|
Version: | 1.0 → 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 by , 16 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:4 by , 16 years ago
Resolution: | fixed |
---|---|
Status: | closed → 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 by , 16 years ago
Resolution: | → fixed |
---|---|
Status: | reopened → 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.
comment:6 by , 16 years ago
Resolution: | fixed |
---|---|
Status: | closed → 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 by , 16 years ago
This is better, but two comments:
- The versionadded flag doesn't render - you can't have a versionadded in between bullet points like you have used
- 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.
comment:8 by , 16 years ago
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 by , 16 years ago
Summary: | Improve documentation on raw SQL queries → Improve documentation on PostgreSQL transactions, exceptions and savepoints |
---|
comment:10 by , 16 years ago
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.
comment:11 by , 16 years ago
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 by , 15 years ago
Owner: | changed from | to
---|---|
Status: | reopened → assigned |
comment:13 by , 15 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
Improve documentation on raw SQL queries