Opened 9 years ago

Last modified 8 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

http://i5.imageban.ru/out/2015/12/20/58dc28d049b669a5633a8a7b9643234b.png

Attachments (4)

django_master.patch​ (1.2 KB ) - added by Artyem Klimenko 9 years ago.
django_stable_1.8.patch​ (1.2 KB ) - added by Artyem Klimenko 9 years ago.
django_stable_1.9.patch​ (1.2 KB ) - added by Artyem Klimenko 9 years ago.
django_master_v2.patch​ (3.1 KB ) - added by Artyem Klimenko 9 years ago.

Download all attachments as: .zip

Change History (13)

by Artyem Klimenko, 9 years ago

Attachment: django_master.patch​ added

by Artyem Klimenko, 9 years ago

Attachment: django_stable_1.8.patch​ added

by Artyem Klimenko, 9 years ago

Attachment: django_stable_1.9.patch​ added

comment:1 by Shai Berger, 9 years ago

This is the expected behavior, as far as I can see. Can you elaborate on what you expected to happen?

comment:2 by Shai Berger, 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.

in reply to:  2 comment:3 by Artyem Klimenko, 9 years ago

Cc: aklim007@… 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 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.

​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 transaktion.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 in order and there is atomic.

Version 0, edited 9 years ago by Artyem Klimenko (next)

comment:4 by Shai Berger, 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 Artyem Klimenko, 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 Artyem Klimenko, 9 years ago

Attachment: django_master_v2.patch​ added

comment:6 by Shai Berger, 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 Tim Graham, 9 years ago

Triage Stage: Unreviewed → Someday/Maybe

Bumping to "Someday/Maybe" pending the outcome of the ​mailing list discussion.

comment:8 by Collin Anderson, 6 years ago

Patch needs improvement: set

comment:9 by Ülgen Sarıkavak, 8 months ago

Cc: Ülgen Sarıkavak added
Note: See TracTickets for help on using tickets.
Back to Top