[Django] #30382: force_insert not respected when saving parents
Django
-------------------------------------+-------------------------------------
Reporter: Phill | Owner: nobody
Tornroth |
Type: Bug | Status: new
Component: Database | Version: 2.2
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
We're using non-abstract model inheritance (table per model class) and
issuing our own primary keys. When saving we pass force_insert=True to
prevent the extra UPDATE statement that precedes the INSERT. The
force_insert flag is respected on the child table but not on the parent.
So given:
{{{
class ParentModel(models.Model):
id = models.BigIntegerField(primary_key=True)
class ChildModel(ParentModel):
pass
ChildModel(id=1).save(force_insert=True)
}}}
We'll see queries:
1. UPDATE app_parentmodel (no rows affected)
2. INSERT app_parentmodel
3. INSERT app_childmodel
This is because Model.save_base doesn't pass force_insert along to
Model._save_parents, and onto Model._save_table. Doing so would prevent
the extra UPDATE and respect the spirit of the force_insert feature. This
is a change I've made locally and seems to work / is pretty
straightforward. I'm curious though if there's intent behind not carrying
the force_insert functionality through for parents. I couldn't find any
discussion on the topic.
For context about why this is particularly problematic in our case --
we're using MySQL w/ Innodb and Innodb will take out different exclusive
locks for the UPDATE and INSERT in question -- so if you're creating new
ChildModel instances in parallel you can get deadlocks when multiple
threads issue query #1 and then need a lock with insert intention in order
to get #2.
--
Ticket URL: <https://code.djangoproject.com/ticket/30382>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: Phill Tornroth | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* version: 2.2 => master
* stage: Unreviewed => Accepted
Comment:
Sounds reasonable.
--
Ticket URL: <https://code.djangoproject.com/ticket/30382#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: Phill Tornroth | Owner: nobody
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Phill Tornroth):
I started working on this. As is normal, it's note quite as easy as I
hoped. The manager .create() methods assume force_insert and there's some
previous behavior in tests (and so presumably in the wild) where code
might do:
{{{
parent = ParentModel.objects.create()
child = ChildModel.objects.create(parent=parent)
}}}
The create method will pass force_insert, so the second line fails because
it attempts to insert the parent again. A prior attempt from @akaariai
suggested breaking this behavior (which I'm game for, if the team
suggests). I can't think of a reasonable alternative that doesn't involve
some other backwards-incompatible change (like making force_insert an
explicit argument to create).
Here's the unmerged commit from @akaraaia:
https://github.com/akaariai/django/commit/57384c7936fbd8d760a36c47a41ecef18e451308
From the (effectively duplicate) ticket:
https://code.djangoproject.com/ticket/18305
--
Ticket URL: <https://code.djangoproject.com/ticket/30382#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: Phill Tornroth | Owner: nobody
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by KESHAV KUMAR):
I want to work on this ticket. Can I get some help about it ?
--
Ticket URL: <https://code.djangoproject.com/ticket/30382#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: Phill Tornroth | Owner: nobody
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => fixed
Comment:
This issue seems to be closed on
https://github.com/django/django/commit/6b4834952dcce0db5cbc1534635c00ff8573a6d8
. Tested this with the current main with the following test.
{{{
class Counter(models.Model):
name = models.CharField(max_length=10)
value = models.IntegerField()
class SubCounter(Counter):
pass
def test_force_insert_on_im(self):
a = Counter(id=1, name="Counter", value=3).save()
a = SubCounter(id=1, name="Subcounter",
value=2).save(force_insert=True)
cntr = Counter.objects.first()
subcntr = SubCounter.objects.first()
self.assertEqual(subcntr.name, cntr.name)
self.assertEqual(subcntr.value, cntr.value)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30382#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: Phill Tornroth | Owner: nobody
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: closed => new
* resolution: fixed =>
Comment:
This is not fixed. `force_insert` is not passed to `_save_parents()` etc.
Also, this ticket was accepted in 2019 so I'm not sure how it could be
fixed 6 years earlier 🤔 Please don't close tickets without sending PR
with tests confirming that they have been fixed.
--
Ticket URL: <https://code.djangoproject.com/ticket/30382#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Phill Tornroth | Owner: nobody
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Akash Kumar Sen):
Sorry about that. Just wanted to add the comment, I thought It would
suggest about closing instead of actually closing them . will try to look
into the {{{_save_parents()}}} method
--
Ticket URL: <https://code.djangoproject.com/ticket/30382#comment:6>
Django
-------------------------------------+-------------------------------------
| Kumar Sen
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => Akash Kumar Sen
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/30382#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Phill Tornroth | Owner: Akash
| Kumar Sen
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Akash Kumar Sen):
PR https://github.com/django/django/pull/16830
--
Ticket URL: <https://code.djangoproject.com/ticket/30382#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Phill Tornroth | Owner: Akash
| Kumar Sen
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Natalia Bidart):
Hello phill-tornroth,
I'm trying to reproduce the reported issue in order to properly review the
proposed PR. I'm not able to generate the three queries you mentioned
(`UPDATE`, `INSERT`, `INSERT`). What I see using sqlite, PG or MySQL is
the same thing:
{{{
1. SELECT 1 AS `a` FROM `ticket_30382_parentmodel` WHERE
`ticket_30382_parentmodel`.`id` = 1 LIMIT 1
2. INSERT INTO `ticket_30382_parentmodel` (`id`) VALUES (1)
3. INSERT INTO `ticket_30382_childmodel` (`parentmodel_ptr_id`) VALUES (1)
}}}
Could you share your setup to provide instructions in how to reproduce the
reported problem?
My `models.py`:
{{{
from django.db import models
class ParentModel(models.Model):
id = models.BigIntegerField(primary_key=True)
class ChildModel(ParentModel):
pass
}}}
My `tests.py`:
{{{
from django.test import TestCase
from .models import ChildModel
class Ticke30382TestCase(TestCase):
def test_force_insert_save(self):
with self.assertNumQueries(0):
ChildModel(id=1).save(force_insert=True)
}}}
My test run:
{{{
======================================================================
FAIL: test_force_insert_save
(ticket_30382.tests.Ticke30382TestCase.test_force_insert_save)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/nessita/fellowship/projectfromrepo/ticket_30382/tests.py",
line 9, in test_force_insert_save
with self.assertNumQueries(0):
File "/home/nessita/.virtualenvs/djangodev/lib/python3.11/site-
packages/django/test/testcases.py", line 99, in __exit__
self.test_case.assertEqual(
AssertionError: 3 != 0 : 3 queries executed, 0 expected
Captured queries were:
1. SELECT 1 AS `a` FROM `ticket_30382_parentmodel` WHERE
`ticket_30382_parentmodel`.`id` = 1 LIMIT 1
2. INSERT INTO `ticket_30382_parentmodel` (`id`) VALUES (1)
3. INSERT INTO `ticket_30382_childmodel` (`parentmodel_ptr_id`) VALUES (1)
----------------------------------------------------------------------
Ran 1 test in 0.038s
}}}
Thanks!
--
Ticket URL: <https://code.djangoproject.com/ticket/30382#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: Phill Tornroth | Owner: Akash
| Kumar Sen
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Akash Kumar Sen):
Hi Natalia,
Try replacing your {{{models.py}}} with the following and it would
probably work. There seems to be another bug that goes beyond reported
here. When the primary key field is the only field in the model it
generates an extra select statement. Created another ticket #34549 for
that. Check for better explanation.
{{{
class ParentModel(models.Model):
id = models.BigIntegerField(primary_key=True)
name = models.CharField(max_length=10)
class ChildModel(ParentModel):
pass
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30382#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: Phill Tornroth | Owner: Akash
| Kumar Sen
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Natalia Bidart):
Thank you Akash, your comment helped me reproduce the reported issue. In
summary, a Parent model with at least two fields, `id` and `name` for
example, would generate these queries for
`ChildModel(id=1).save(force_insert=True)`:
{{{
1. UPDATE "ticket_30382_parentmodel" SET "name" = '' WHERE
"ticket_30382_parentmodel"."id" = 1
2. INSERT INTO "ticket_30382_parentmodel" ("id", "name") VALUES (1, '')
3. INSERT INTO "ticket_30382_childmodel" ("parentmodel_ptr_id") VALUES (1)
}}}
I also added a test for the alternate code:
{{{
parent = ParentModel.objects.create(id=1)
ChildModel.objects.create(parentmodel_ptr=parent)
}}}
which generates these queries:
{{{
1. INSERT INTO "ticket_30382_parentmodel" ("id", "name") VALUES (1, '')
2. UPDATE "ticket_30382_parentmodel" SET "name" = '' WHERE
"ticket_30382_parentmodel"."id" = 1
3. INSERT INTO "ticket_30382_childmodel" ("parentmodel_ptr_id") VALUES (1)
}}}
When testing this same code with the **proposed PR**, **the first case
gets fixed as expected**, these are the queries I get for the reported
code (so YEY):
{{{
1. INSERT INTO "ticket_30382_parentmodel" ("id", "name") VALUES (1, '')
2. INSERT INTO "ticket_30382_childmodel" ("parentmodel_ptr_id") VALUES (1)
}}}
**But** for the second code snippet (create parent and then create child)
I get an **extra query** (so HUM). Is this expected? I wouldn't think
so...
{{{
1. INSERT INTO "ticket_30382_parentmodel" ("id", "name") VALUES (1, '')
2. UPDATE "ticket_30382_parentmodel" SET "name" = '' WHERE
"ticket_30382_parentmodel"."id" = 1
3. SELECT 1 AS "a" FROM "ticket_30382_childmodel" WHERE
"ticket_30382_childmodel"."parentmodel_ptr_id" = 1 LIMIT 1
4. INSERT INTO "ticket_30382_childmodel" ("parentmodel_ptr_id") VALUES (1)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30382#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: Phill Tornroth | Owner: Akash
| Kumar Sen
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Akash Kumar Sen):
Hi Natalia,
Finally found a way to optimize the extra query :). Updated the patch
accordingly.
--
Ticket URL: <https://code.djangoproject.com/ticket/30382#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: Phill Tornroth | Owner: Akash
| Kumar Sen
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/30382#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: Phill Tornroth | Owner: Akash
| Kumar Sen
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/30382#comment:14>
Django
-------------------------------------+-------------------------------------
Reporter: Phill Tornroth | Owner: Akash
| Kumar Sen
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
Comment:
Removed inspection of a stack frame but the solution still continues to be
hacky.
--
Ticket URL: <https://code.djangoproject.com/ticket/30382#comment:15>
Django
-------------------------------------+-------------------------------------
Reporter: Phill Tornroth | Owner: Akash
| Kumar Sen
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/30382#comment:16>
Django
-------------------------------------+-------------------------------------
Reporter: Phill Tornroth | Owner: Akash
| Kumar Sen
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/30382#comment:17>
Django
-------------------------------------+-------------------------------------
Reporter: Phill Tornroth | Owner: Akash
| Kumar Sen
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Sarah Boyce):
Link to the google group discussion: https://groups.google.com/g/django-
developers/c/YJBoOapR_gQ/m/s5AWbN-PAQAJ
--
Ticket URL: <https://code.djangoproject.com/ticket/30382#comment:18>
Django
-------------------------------------+-------------------------------------
Reporter: Phill Tornroth | Owner: Akash
| Kumar Sen
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/30382#comment:19>
Django
-------------------------------------+-------------------------------------
Reporter: Phill Tornroth | Owner: Akash
| Kumar Sen
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
Comment:
- Removed support for single {{{force_insert=Modelbase}}} user have to
wrap it into a tuple. Like {{{force_insert=(Modelbase,)}}}
- Optimize the code by calling validation only if the class have parents
to be inserted.
- Reduce the number of models in tests.
--
Ticket URL: <https://code.djangoproject.com/ticket/30382#comment:20>
Django
-------------------------------------+-------------------------------------
Reporter: Phill Tornroth | Owner: Akash
| Kumar Sen
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"ce204bed7f59e34a9ec226e1699db257ec33da30" ce204bed]:
{{{
#!CommitTicketReference repository=""
revision="ce204bed7f59e34a9ec226e1699db257ec33da30"
Refs #30382 -- Added more tests for using force_insert with model
inheritance.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30382#comment:21>
Django
-------------------------------------+-------------------------------------
Reporter: Phill Tornroth | Owner: Akash
| Kumar Sen
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/30382#comment:22>
Django
-------------------------------------+-------------------------------------
Reporter: Phill Tornroth | Owner: Akash
| Kumar Sen
Component: Database layer | Version: dev
(models, ORM) |
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"a40b0103bccb8216c944188d329d8ea5eceb7e92" a40b010]:
{{{
#!CommitTicketReference repository=""
revision="a40b0103bccb8216c944188d329d8ea5eceb7e92"
Fixed #30382 -- Allowed specifying parent classes in force_insert of
Model.save().
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30382#comment:23>