Opened 9 years ago
Last modified 9 months ago
#25955 new Bug
FK constraints are not checked at the end of nested atomic blocks
| Reported by: | Artyem Klimenko | Owned by: | nobody |
|---|---|---|---|
| Component: | Database layer (models, ORM) | Version: | dev |
| Severity: | Normal | Keywords: | DEFERRED transaction.atomic savepoint postgresql |
| Cc: | aklim007@…, Ülgen Sarıkavak | Triage Stage: | Someday/Maybe |
| Has patch: | yes | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | yes |
| Easy pickings: | no | UI/UX: | no |
Description
Create 2 models:
class Model1(models.Model): pass class Model2(models.Model): model1 = models.ForeignKey(Model1)
foreign key in DB:
CONSTRAINT zabbix_model2_model1_id_4d6fe5808c3891b4_fk_zabbix_model1_id FOREIGN KEY (model1_id) REFERENCES zabbix_model1 (id) MATCH SIMPLE ON UPDATE NO ACTION ON DELETE NO ACTION DEFERRABLE INITIALLY DEFERRED -- INITIALLY DEFERRED !!!!!!!!!!!!!!!!!!! -- DEFERRABLE constraints set to DEFERRED (INITIALLY DEFERRED or via SET CONSTRAINTS) are checked after each transaction (not savepoint release).
add row in model1:
>>> Model1().save() >>> Model1.objects.values() [{'id': 1}]
view:
@transaction.atomic() def _test(request): model1_ids = [ 1, 2, # incorrect ForeignKey 1. ] for model1_id in model1_ids: try: with transaction.atomic(): Model2(model1_id=model1_id).save() except IntegrityError as e: # magic pass return render(request, 'test_template.html')
Request Method: GET
Request URL: http://127.0.0.1:8000/test
Django Version: 1.8.6
Exception Type: IntegrityError
Exception Value:
insert or update on table "zabbix_model2" violates foreign key constraint "zabbix_model2_model1_id_4d6fe5808c3891b4_fk_zabbix_model1_id"
DETAIL: Key (model1_id)=(2) is not present in table "zabbix_model1".
Exception Location: /home/its/venv/lib/python3.4/site-packages/django/db/backends/base/base.py in _commit, line 142
Python Executable: /home/its/venv/bin/python
Python Version: 3.4.2
Attachments (4)
Change History (13)
by , 9 years ago
| Attachment: | django_master.patch added |
|---|
by , 9 years ago
| Attachment: | django_stable_1.8.patch added |
|---|
by , 9 years ago
| Attachment: | django_stable_1.9.patch added |
|---|
comment:1 by , 9 years ago
follow-up: 3 comment:2 by , 9 years ago
| Resolution: | → wontfix |
|---|---|
| Status: | new → closed |
| Summary: | INITIALLY DEFERRED, nested transaction.atomic, postgresql → FK constraints are not checked at the end of nested atomic blocks |
Oh, looking at the patch, I see you expect deferred constraints to be checked at the end of nested atomic blocks.
Unless you can point some place in the documentation where such behavior is described, I'd call this a huge backward incompatibility. If you can show such a place, please re-open.
comment:3 by , 9 years ago
| Cc: | added |
|---|---|
| Resolution: | wontfix |
| Status: | closed → new |
Replying to shaib:
Oh, looking at the patch, I see you expect deferred constraints to be checked at the end of nested
atomicblocks.
Unless you can point some place in the documentation where such behavior is described, I'd call this a huge backward incompatibility. If you can show such a place, please re-open.
https://docs.djangoproject.com/en/1.9/topics/db/transactions/#controlling-transactions-explicitly
Code highlighting:
from django.db import IntegrityError, transaction @transaction.atomic def viewfunc(request): create_parent() try: with transaction.atomic(): generate_relationships() except IntegrityError: handle_exception() add_children()
In mysql it works fine.
The current behavior of Postgres violates the logic transaction.atomic.
To work correctly, I have to think about nesting transaktion.atomic.
If the function is wrapped in a atomic, atomic cause within another unit will conduct one another more.
If the unit is completed successfully, I expect that everything is all right and there is atomic.
comment:4 by , 9 years ago
The docs you cite say very explicitly what Django does with transactions and savepoints, and the behavior is in complete accordance. They do imply what you took from them, and the point that the documentation's example doesn't really work under postgresql is strong. However, forcing constraint checks at the end of every nested atomic block is likely to break a lot of existing code, so the fix you suggested cannot be accepted; not for master, and most certainly not for the released branches.
We can treat this two ways: One is as a behavior problem; to fix it as such, you'll need to provide an upgrade path -- that is, a method for users to find out that they have a problem and fix it or work around it. Also, no change in transactional behavior is likely to be accepted without a discussion on the DevelopersMailingList, so if you want to see this kind of change happen, please write to the list.
The other is as a documentation problem: Make the example create integrity errors that are not about foreign keys, and perhaps add a paragraph explaining that deferred constraints are not necessarily checked at the end of atomic blocks.
comment:5 by , 9 years ago
How about adding a parameter in OPTIONS?
Code highlighting:
DATABASES = { 'default': { 'ENGINE': 'django.db.backends.postgresql_psycopg2', # ... 'OPTIONS': { 'savepoint_check_constraints': True } }, }
full backward compatibility
add patch
by , 9 years ago
| Attachment: | django_master_v2.patch added |
|---|
comment:6 by , 9 years ago
As I said before, changes in transactional behavior will not happen without discussion on the DevelopersMailingList.
Also, you may benefit from reading https://docs.djangoproject.com/en/stable/intro/contributing/ and https://docs.djangoproject.com/en/stable/internals/contributing/writing-code/
comment:7 by , 9 years ago
| Triage Stage: | Unreviewed → Someday/Maybe |
|---|
Bumping to "Someday/Maybe" pending the outcome of the mailing list discussion.
comment:8 by , 6 years ago
| Patch needs improvement: | set |
|---|
comment:9 by , 9 months ago
| Cc: | added |
|---|
This is the expected behavior, as far as I can see. Can you elaborate on what you expected to happen?