Opened 8 months ago

Closed 7 months ago

Last modified 7 months ago

#30953 closed Bug (fixed)

Model Inheritance and select_for_update with 'of'

Reported by: Abhijeet Viswa Owned by: felixxm
Component: Database layer (models, ORM) Version: 2.1
Severity: Release blocker Keywords: select_for_update mti model inheritance
Cc: Abhijeet Viswa Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Consider two models A and B with B being a sub model of A. If I have a select and update that looks like this (assume it all happens inside an atomic transaction):

b = B.objects.filter(pk=1).select_for_update(of=('self'))
b = b[0]  # Evaluate the query
b.some_field_of_b = b.some_field_of_b + 1
b.save()

This will cause race conditions. The generated SQL will only lock the table of A (the super-model) and not of B. As you can see, the field being updated is of B and not of A. Even though the SELECT .. FOR UPDATE OF might block, the queryset returned will not contain the updated data of the table of B because the snapshot will not include the updated data (before a lock is even acquired).

When the of parameter for select_for_update is supplied, the tables locked should include those of the sub-models of the of fields.

Change History (15)

comment:1 Changed 8 months ago by Abhijeet Viswa

Owner: changed from nobody to Abhijeet Viswa
Status: newassigned

comment:2 Changed 8 months ago by Simon Charette

Keywords: select_for_update mti model inheritance added; race conditions psql concurrency db removed
Triage Stage: UnreviewedAccepted
Version: 2.2master

The generated SQL will only lock the table of A (the super-model) and not of B.

I confirmed this is the case, self always use the parent model table instead of the current one.

I'm not convinced we should make select_for_update(of=('self',)) select all inherited tables though as that would prevents selecting only the current leaf model table. IMO the code should only be changed to make self point to the queryset's model table as documented.

I suggest we also enhance the documentation to mention that parent links should be included in of when this is desired instead.

In your case that would mean of=('self', 'a_ptr') to select both tables for update.

comment:3 Changed 8 months ago by Abhijeet Viswa

Cc: Abhijeet Viswa added
Has patch: set

comment:4 Changed 8 months ago by Abhijeet Viswa

Created a patch pull request on GitHub: 12082.

I believe this bug qualifies under data loss bugs as the unexpected behavior caused due to this bug could cause race conditions (as was the case for me for my project). I've added release notes to 2.2.8. However, I'm not sure if it should be added to 3.0 or 3.1 and should be backported to 2.1. Please guide me on this so that I can modify the patch appropriately.

Last edited 8 months ago by Abhijeet Viswa (previous) (diff)

comment:5 Changed 8 months ago by felixxm

Severity: NormalRelease blocker
Version: master2.1

I agree this qualifies for a backport as a data loss bug.

comment:6 Changed 8 months ago by felixxm

Needs tests: set
Patch needs improvement: set

comment:7 Changed 7 months ago by felixxm

Needs tests: unset
Owner: changed from Abhijeet Viswa to felixxm
Patch needs improvement: unset

comment:8 Changed 7 months ago by GitHub <noreply@…>

Resolution: fixed
Status: assignedclosed

In 0107e3d1:

Fixed #30953 -- Made select_for_update() lock queryset's model when using "self" with multi-table inheritance.

Thanks Abhijeet Viswa for the report and initial patch.

comment:9 Changed 7 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In f4ed6800:

[3.0.x] Fixed #30953 -- Made select_for_update() lock queryset's model when using "self" with multi-table inheritance.

Thanks Abhijeet Viswa for the report and initial patch.
Backport of 0107e3d1058f653f66032f7fd3a0bd61e96bf782 from master

comment:10 Changed 7 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In 6cf3b6f5:

[2.2.x] Fixed #30953 -- Made select_for_update() lock queryset's model when using "self" with multi-table inheritance.

Thanks Abhijeet Viswa for the report and initial patch.
Backport of 0107e3d1058f653f66032f7fd3a0bd61e96bf782 from master

comment:11 Changed 7 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In 39e39d0a:

Refs #30953 -- Added 2.1.15 release note for 0107e3d1058f653f66032f7fd3a0bd61e96bf782.

comment:12 Changed 7 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In db0cc4a:

[3.0.x] Refs #30953 -- Added 2.1.15 release note for 0107e3d1058f653f66032f7fd3a0bd61e96bf782.

Backport of 39e39d0ac1b720e7460ec8ccf45926c78edb2047 from master

comment:13 Changed 7 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In 70311e1:

[2.2.x] Refs #30953 -- Added 2.1.15 release note for 0107e3d1058f653f66032f7fd3a0bd61e96bf782.

Backport of 39e39d0ac1b720e7460ec8ccf45926c78edb2047 from master

comment:14 Changed 7 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In 015fab76:

[2.1.x] Fixed #30953 -- Made select_for_update() lock queryset's model when using "self" with multi-table inheritance.

Thanks Abhijeet Viswa for the report and initial patch.

Backport of 0107e3d1058f653f66032f7fd3a0bd61e96bf782 from master.

comment:15 Changed 7 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In f57f81a7:

[2.1.x] Refs #30953 -- Added 2.1.15 release note for 0107e3d1058f653f66032f7fd3a0bd61e96bf782.

Backport of 39e39d0ac1b720e7460ec8ccf45926c78edb2047 from master

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