Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#30953 closed Bug (fixed)

Model Inheritance and select_for_update with 'of'

Reported by: Abhijeet Viswa Owned by: Mariusz Felisiak
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 by Abhijeet Viswa, 4 years ago

Owner: changed from nobody to Abhijeet Viswa
Status: newassigned

comment:2 by Simon Charette, 4 years ago

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 by Abhijeet Viswa, 4 years ago

Cc: Abhijeet Viswa added
Has patch: set

comment:4 by Abhijeet Viswa, 4 years ago

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 4 years ago by Abhijeet Viswa (previous) (diff)

comment:5 by Mariusz Felisiak, 4 years ago

Severity: NormalRelease blocker
Version: master2.1

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

comment:6 by Mariusz Felisiak, 4 years ago

Needs tests: set
Patch needs improvement: set

comment:7 by Mariusz Felisiak, 4 years ago

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

comment:8 by GitHub <noreply@…>, 4 years ago

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 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

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 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

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 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

In 39e39d0a:

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

comment:12 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

In db0cc4a:

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

Backport of 39e39d0ac1b720e7460ec8ccf45926c78edb2047 from master

comment:13 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

In 70311e1:

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

Backport of 39e39d0ac1b720e7460ec8ccf45926c78edb2047 from master

comment:14 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

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 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

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