Opened 3 years ago

Closed 2 years ago

#17341 closed Bug (fixed)

Model.save() commits transactions after every parent class save

Reported by: akaariai Owned by: Anssi Kääriäinen <akaariai@…>
Component: Database layer (models, ORM) Version: 1.3
Severity: Normal Keywords:
Cc: anssi.kaariainen@…, sebastian.goll@…, charette.s@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

The bug is that when running outside transaction management, Django does a save() after every update/insert. In multitable inheritance, it is possible (although rare) that this will result in half-saved objects. For example, using postgresql, and model inheritance I get these queries:

BEGIN
SELECT (1) AS "a" FROM "parent" WHERE "parent"."id" = 1  LIMIT 1
UPDATE "parent" SET "f1" = 1 WHERE "parent"."id" = 1 
COMMIT -- Why commit here?
BEGIN
INSERT INTO "child" ("parent_ptr_id", "pk2") VALUES (1, NULL)
ERROR:  null value in column "pk2" violates not-null constraint

Now I have committed a half-update. The attached patch gets rid of the COMMIT/BEGIN in the middle.

I found this while hacking on #17332. It has more cleanups for .save (for example proxy handling), but I thought this deserves its own ticket.

Attachments (3)

17341.diff (1.1 KB) - added by akaariai 3 years ago.
17341.2.diff (11.0 KB) - added by akaariai 3 years ago.
Cleanup of base_save() + tests
17341.3.diff (12.3 KB) - added by akaariai 3 years ago.

Download all attachments as: .zip

Change History (13)

Changed 3 years ago by akaariai

comment:1 Changed 3 years ago by akaariai

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

As #17332 was marked as DDN (fully agreed), I think the cleanup of .save() included in the patch attached to that ticket should be moved to this ticket.

There are two things needing cleanup:

  • The position of commit_unless_managed, which this ticket addresses directly
  • The logic about handling proxy models. The problem here is that it is hard to follow what is happening. So hard, that I am pretty sure the last if condition before the actual save
    if not meta.is_proxy:
        do the actual save
    

is actually not needed at all. A proxy model can never reach this stage, see the "if meta.is_proxy: return" above.

I can write a patch incorporating this change. If piggy-bagging the proxy change is not wanted into this ticket, I can open another ticket. Or just leave it as is if that is wanted (IMHO bad idea...).

comment:2 Changed 3 years ago by aaugustin

  • Needs tests set

Yes, this makes sense.

comment:3 Changed 3 years ago by aaugustin

  • Triage Stage changed from Unreviewed to Accepted

comment:4 Changed 3 years ago by akaariai

Ok, here is a patch. In my opinion it does make it easier to understand what is going on in the saving process. I have tried to make sure saving works just like before. The only behavioral change should be committing only once, after all tables have been saved. At least the tests agree I haven't broken anything.

There is one thing I don't like, and that is the parameter name "topmost_concrete". It is needed so that we know to send signals only once, after the last concrete (non-proxy) model's table is saved. Better names (or better logic) welcome. Before this was done using origin, which was self.__class__, until first concrete model was seen, and after that it was None. The information in origin was not needed for anything else than checking when to send signals.

Changed 3 years ago by akaariai

Cleanup of base_save() + tests

comment:5 Changed 3 years ago by akaariai

  • Needs tests unset

I realized there is one more plain old bug hidden in there: force_insert & force_update are not passed down the inheritance chain. There might be some reason for not doing that for multitable inheritance (does it mean force_insert all the tables, or force_insert just the topmost table?). But for proxy models this is a clear bug.

I now think it might be better to write the cleanup as:

def save_base(self, ...):
    # Do state setup & some special pre & post things unique to first save_base call.
    self._save_base(...)

def _save_base(...):
    # Do just inheritance chain traveling + actual saves here. No signals, no transactions.

The reason for the additional method is that the first save_base call is special. The additional method makes it explicit. It is easy to see that we only send the signals for the topmost model. I could not easily see that in the current trunk version. Patch attached. Me likes this version, but maybe this is getting overly cute...

In the latest patch, actual behavioral changes are:

  • commit_unless_managed is called only once, after all tables have been saved.
  • force_update & force_insert do have an effect for proxy models. As before, these are not passed down to multitable inheritance chains.

These changes have tests in the patch. All tests passed on SQLite.

The rest is just cleanup. There should not be any changes in how the per-table saving is done (the large if not meta.proxy: branch in trunk code is just reindented).

I wonder if I am once again extending the scope of the ticket too much. Maybe rename this to cleanup of model.save_base()? If not, just say and I will create a separate ticket for the general cleanup and leave this ticket just for the transaction case. In that case, the first submitted patch is the way to go.

Last: is it intentional that force_insert & force_update are not passed down the inheritance chain in multitable inheritance case?

Changed 3 years ago by akaariai

comment:6 Changed 3 years ago by sebastian

  • Cc sebastian.goll@… added

comment:7 Changed 3 years ago by charettes

  • Cc charette.s@… added

comment:8 Changed 2 years ago by aaugustin

  • Owner changed from nobody to aaugustin
  • Status changed from new to assigned

comment:9 Changed 2 years ago by aaugustin

  • Owner aaugustin deleted
  • Patch needs improvement set
  • Status changed from assigned to new

The patch needs to be rewritten with transaction.atomic.

Unfortunately it's moving a lot of code around and I can't see exactly where to inject the atomic wrapper.

comment:10 Changed 2 years ago by Anssi Kääriäinen <akaariai@…>

  • Owner set to Anssi Kääriäinen <akaariai@…>
  • Resolution set to fixed
  • Status changed from new to closed

In 6b4834952dcce0db5cbc1534635c00ff8573a6d8:

Fixed #16649 -- Refactored save_base logic

Model.save() will use UPDATE - if not updated - INSERT instead of
SELECT - if found UPDATE else INSERT. This should save a query when
updating, but will cost a little when inserting model with PK set.

Also fixed #17341 -- made sure .save() commits transactions only after
the whole model has been saved. This wasn't the case in model
inheritance situations.

The save_base implementation was refactored into multiple methods.
A typical chain for inherited save is:
save_base()

_save_parents(self)

for each parent:

_save_parents(parent)
_save_table(parent)

_save_table(self)

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