force_insert flag is not passed when saving parents on inherited models.

[Phill Tornroth]

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.

[Mariusz Felisiak]

Sounds reasonable.

[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

[KESHAV KUMAR]

I want to work on this ticket. Can I get some help about it ?

[Akash Kumar Sen]

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)

[Mariusz Felisiak]

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.

[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

[Akash Kumar Sen]

PR ​https://github.com/django/django/pull/16830

[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!

[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

[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)

[Akash Kumar Sen]

Hi Natalia,
Finally found a way to optimize the extra query :). Updated the patch accordingly.

[Akash Kumar Sen]

Removed inspection of a stack frame but the solution still continues to be hacky.

[Sarah Boyce]

Link to the google group discussion: ​https://groups.google.com/g/django-developers/c/YJBoOapR_gQ/m/s5AWbN-PAQAJ

[Akash Kumar Sen]

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

[Mariusz Felisiak <felisiak.mariusz@…>]

In ce204bed:

Refs #30382 -- Added more tests for using force_insert with model inheritance.

[Mariusz Felisiak <felisiak.mariusz@…>]

In a40b010:

Fixed #30382 -- Allowed specifying parent classes in force_insert of Model.save().