#30953 closed Bug (fixed)
Model Inheritance and select_for_update with 'of'
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 , 5 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:2 by , 5 years ago
Keywords: | select_for_update mti model inheritance added; race conditions psql concurrency db removed |
---|---|
Triage Stage: | Unreviewed → Accepted |
Version: | 2.2 → master |
comment:3 by , 5 years ago
Cc: | added |
---|---|
Has patch: | set |
comment:4 by , 5 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.
comment:5 by , 5 years ago
Severity: | Normal → Release blocker |
---|---|
Version: | master → 2.1 |
I agree this qualifies for a backport as a data loss bug.
comment:6 by , 5 years ago
Needs tests: | set |
---|---|
Patch needs improvement: | set |
comment:7 by , 5 years ago
Needs tests: | unset |
---|---|
Owner: | changed from | to
Patch needs improvement: | unset |
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 makeself
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.