Opened 5 years ago

Closed 13 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:

  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.

Change History (23)

comment:1 by Mariusz Felisiak, 5 years ago

Summary: force_insert not respected when saving parentsforce_insert flag is not passed when saving parents on inherited models.
Triage Stage: UnreviewedAccepted
Version: 2.2master

Sounds reasonable.

comment:2 by Phill Tornroth, 5 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:3 by KESHAV KUMAR, 5 years ago

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

comment:4 by Akash Kumar Sen, 15 months ago

Resolution: fixed
Status: newclosed

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 Mariusz Felisiak, 15 months ago

Resolution: fixed
Status: closednew

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 Akash Kumar Sen, 15 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 Akash Kumar Sen, 15 months ago

Owner: changed from nobody to Akash Kumar Sen
Status: newassigned

comment:9 by Natalia Bidart, 15 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 Akash Kumar Sen, 15 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
Last edited 15 months ago by Akash Kumar Sen (previous) (diff)

comment:11 by Natalia Bidart, 15 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 Akash Kumar Sen, 15 months ago

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

comment:13 by Akash Kumar Sen, 14 months ago

Has patch: set

comment:14 by Mariusz Felisiak, 14 months ago

Patch needs improvement: set

comment:15 by Akash Kumar Sen, 14 months ago

Patch needs improvement: unset

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

comment:16 by Mariusz Felisiak, 14 months ago

Patch needs improvement: set

comment:17 by Akash Kumar Sen, 14 months ago

Patch needs improvement: unset

comment:19 by Mariusz Felisiak, 13 months ago

Patch needs improvement: set

comment:20 by Akash Kumar Sen, 13 months ago

Patch needs improvement: unset
  • 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.

comment:21 by Mariusz Felisiak <felisiak.mariusz@…>, 13 months ago

In ce204bed:

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

comment:22 by Mariusz Felisiak, 13 months ago

Triage Stage: AcceptedReady for checkin

comment:23 by Mariusz Felisiak <felisiak.mariusz@…>, 13 months ago

Resolution: fixed
Status: assignedclosed

In a40b010:

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

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