Opened 6 years ago
Closed 18 months ago
#30382 closed Bug (fixed)
force_insert flag is not passed when saving parents on inherited models.
Reported by: | Phill Tornroth | Owned by: | Akash Kumar Sen |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Ready for checkin | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
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:
- UPDATE app_parentmodel (no rows affected)
- INSERT app_parentmodel
- 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.
Change History (23)
comment:1 by , 6 years ago
Summary: | force_insert not respected when saving parents → force_insert flag is not passed when saving parents on inherited models. |
---|---|
Triage Stage: | Unreviewed → Accepted |
Version: | 2.2 → master |
comment:2 by , 6 years ago
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
comment:4 by , 20 months ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
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)
comment:5 by , 20 months ago
Resolution: | fixed |
---|---|
Status: | closed → new |
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.
comment:6 by , 20 months ago
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
comment:7 by , 20 months ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:9 by , 20 months ago
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!
comment:10 by , 19 months ago
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. There are two cases, case-1 : https://code.djangoproject.com/ticket/34549#comment:1 and case-2: https://code.djangoproject.com/ticket/34549#comment:2
class ParentModel(models.Model): id = models.BigIntegerField(primary_key=True) name = models.CharField(max_length=10) class ChildModel(ParentModel): pass
comment:11 by , 19 months ago
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)
comment:12 by , 19 months ago
Hi Natalia,
Finally found a way to optimize the extra query :). Updated the patch accordingly.
comment:13 by , 19 months ago
Has patch: | set |
---|
comment:14 by , 19 months ago
Patch needs improvement: | set |
---|
comment:15 by , 19 months ago
Patch needs improvement: | unset |
---|
Removed inspection of a stack frame but the solution still continues to be hacky.
comment:16 by , 19 months ago
Patch needs improvement: | set |
---|
comment:17 by , 19 months ago
Patch needs improvement: | unset |
---|
comment:18 by , 18 months ago
Link to the google group discussion: https://groups.google.com/g/django-developers/c/YJBoOapR_gQ/m/s5AWbN-PAQAJ
comment:19 by , 18 months ago
Patch needs improvement: | set |
---|
comment:20 by , 18 months ago
Patch needs improvement: | unset |
---|
- Removed support for single
force_insert=Modelbase
user have to wrap it into a tuple. Likeforce_insert=(Modelbase,)
- Optimize the code by calling validation only if the class have parents to be inserted.
- Reduce the number of models in tests.
comment:22 by , 18 months ago
Triage Stage: | Accepted → Ready for checkin |
---|
Sounds reasonable.