Opened 22 months ago

Last modified 22 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: master
Severity: Normal Keywords: DEFERRED transaction.atomic savepoint postgresql
Cc: aklim007@… Triage Stage: Someday/Maybe
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
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 22 months ago.
django_stable_1.8.patch (1.2 KB) - added by Artyem Klimenko 22 months ago.
django_stable_1.9.patch (1.2 KB) - added by Artyem Klimenko 22 months ago.
django_master_v2.patch (3.1 KB) - added by Artyem Klimenko 22 months ago.

Download all attachments as: .zip

Change History (11)

Changed 22 months ago by Artyem Klimenko

Attachment: django_master.patch added

Changed 22 months ago by Artyem Klimenko

Attachment: django_stable_1.8.patch added

Changed 22 months ago by Artyem Klimenko

Attachment: django_stable_1.9.patch added

comment:1 Changed 22 months ago by Shai Berger

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

comment:2 Changed 22 months ago by Shai Berger

Resolution: wontfix
Status: newclosed
Summary: INITIALLY DEFERRED, nested transaction.atomic, postgresqlFK 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 in reply to:  2 Changed 22 months ago by Artyem Klimenko

Cc: aklim007@… added
Resolution: wontfix
Status: closednew

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 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.

Last edited 22 months ago by Artyem Klimenko (previous) (diff)

comment:4 Changed 22 months ago by Shai Berger

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 Changed 22 months ago by Artyem Klimenko

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

Changed 22 months ago by Artyem Klimenko

Attachment: django_master_v2.patch added

comment:6 Changed 22 months ago by Shai Berger

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 Changed 22 months ago by Tim Graham

Triage Stage: UnreviewedSomeday/Maybe

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

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