[Django] #30375: Use "NO KEY" when doing select_for

Django

unread,

Apr 16, 2019, 3:19:12 PM4/16/19

to django-...@googlegroups.com

#30375: Use "NO KEY" when doing select_for_update for PostgreSQL
-------------------------------------+-------------------------------------
Reporter: mudetz | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database | Version: 1.11
layer (models, ORM) | Keywords: postgres, lock,
Severity: Normal | database, operation
Triage Stage: | Has patch: 1
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
Currently Django compiles select for update into one of
- FOR UPDATE
- FOR UPDATE NOWAIT
- FOR UPDATE SKIP LOCKED

All of these acquire a lock which conflicts with `KEY SHARE` locks. This
means that updates on tables which references a row locked by `FOR UPDATE`
will have to wait until the lock is released. This is to avoid any
unexpected changes on `PRIMARY KEY` fields but, **in Django, primary key
fields are read-only, so locking them makes no sense**.

There is no need to acquire these kind of locks, instead, we should (at
least be able to) use
- FOR NO KEY UPDATE
- FOR NO KEY UPDATE NOWAIT
- FOR NO KEY UPDATE SKIP LOCKED

which can be easily done by overriding BaseOperation.for_update_sql in
Postgres DatabaseOperation

{{{
def for_update_sql(self, nowait=False, skip_locked=False):
"""
Returns the FOR UPDATE SQL clause to lock rows for an update
operation.
"""
if nowait:
return 'FOR NO KEY UPDATE NOWAIT'
elif skip_locked:
return 'FOR NO KEY UPDATE SKIP LOCKED'
else:
return 'FOR NO KEY UPDATE'
}}}

Not doing so causes lock contention in our use case.

--
Ticket URL: <https://code.djangoproject.com/ticket/30375>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

Django

unread,

Apr 16, 2019, 5:20:35 PM4/16/19

to django-...@googlegroups.com

#30375: Use "NO KEY" when doing select_for_update for PostgreSQL
-------------------------------------+-------------------------------------
Reporter: mudetz | Owner: nobody
Type: | Status: new
Cleanup/optimization |

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by mudetz):

* version: 1.11 => 2.2

Old description:

New description:

Currently Django compiles select for update into one of
- FOR UPDATE
- FOR UPDATE NOWAIT
- FOR UPDATE SKIP LOCKED

All of these acquire a lock which conflicts with `KEY SHARE` locks. This
means that updates on tables which references a row locked by `FOR UPDATE`
will have to wait until the lock is released. This is to avoid any
unexpected changes on `PRIMARY KEY` fields but, **in Django, primary key
fields are read-only, so locking them makes no sense**.

There is no need to acquire these kind of locks, instead, we should (at
least be able to) use
- FOR NO KEY UPDATE
- FOR NO KEY UPDATE NOWAIT
- FOR NO KEY UPDATE SKIP LOCKED

which can be easily done by overriding BaseOperation.for_update_sql in
Postgres DatabaseOperation

For 1.11

{{{
def for_update_sql(self, nowait=False, skip_locked=False):
"""
Returns the FOR UPDATE SQL clause to lock rows for an update
operation.
"""
if nowait:
return 'FOR NO KEY UPDATE NOWAIT'
elif skip_locked:
return 'FOR NO KEY UPDATE SKIP LOCKED'
else:
return 'FOR NO KEY UPDATE'
}}}

For 2.2

{{{
def for_update_sql(self, nowait=False, skip_locked=False, of=()):
"""
Return the FOR UPDATE SQL clause to lock rows for an update operation.
"""
return 'FOR NO KEY UPDATE%s%s%s' % (
' OF %s' % ', '.join(of) if of else '',
' NOWAIT' if nowait else '',
' SKIP LOCKED' if skip_locked else '',
)
}}}

Not doing so causes lock contention in our use case.

--

--
Ticket URL: <https://code.djangoproject.com/ticket/30375#comment:1>

Django

unread,

Apr 16, 2019, 5:28:11 PM4/16/19

to django-...@googlegroups.com

#30375: Use "NO KEY" when doing select_for_update for PostgreSQL
-------------------------------------+-------------------------------------

Reporter: Manuel Weitzman | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: master

(models, ORM) |
Severity: Normal | Resolution:
Keywords: postgres, lock, | Triage Stage:
database, operation | Unreviewed

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Simon Charette):

* version: 2.2 => master

Comment:

> ... in Django, primary key fields are read-only, so locking them makes
no sense

That's not true AFAIK. Even if `AutoField` are backed by sequences and
generated on the database side they can still be overridden by developers.
Same thing with non-`AutoField` primary keys (e.g.
`UUIDField(default=uuid.uuid4)`).

On the other hand updating a primary key while holding a lock is really
uncommon so I wouldn't be against adding a `select_for_udate(primary_key)`
parameter that defaults to `False`. We just have to make sure that we
still allow these operations to be performed somehow.

--
Ticket URL: <https://code.djangoproject.com/ticket/30375#comment:2>

Django

unread,

Apr 16, 2019, 6:08:41 PM4/16/19

to django-...@googlegroups.com

#30375: Use "NO KEY" when doing select_for_update for PostgreSQL
-------------------------------------+-------------------------------------

Reporter: Manuel Weitzman | Owner: nobody
Type: | Status: new
Cleanup/optimization |

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Manuel Weitzman):

Replying to [comment:2 Simon Charette]:

> > ... in Django, primary key fields are read-only, so locking them makes
no sense
>
> That's not true AFAIK. Even if `AutoField` are backed by sequences and
generated on the database side they can still be overridden by developers.
Same thing with non-`AutoField` primary keys (e.g.
`UUIDField(default=uuid.uuid4)`).

Then the docs are wrong or at least misleading: ''"The primary key field
is read-only. If you change the value of the primary key on an existing
object and then save it, a new object will be created alongside the old
one."" (https://docs.djangoproject.com/en/2.2/topics/db/models/)

Which means that UPDATEs over PKs are transformed into INSERTs

> On the other hand updating a primary key while holding a lock is really
uncommon so I wouldn't be against adding a `select_for_udate(primary_key)`
parameter that defaults to `False`. We just have to make sure that we
still allow these operations to be performed somehow.

That is possible, I tested today and only a few changes are required to
achieve this behavior. Maybe I could send a PR soon for review?

--
Ticket URL: <https://code.djangoproject.com/ticket/30375#comment:3>

Django

unread,

Apr 16, 2019, 11:53:39 PM4/16/19

to django-...@googlegroups.com

#30375: Use "NO KEY" when doing select_for_update for PostgreSQL
-------------------------------------+-------------------------------------

Reporter: Manuel Weitzman | Owner: nobody
Type: | Status: new
Cleanup/optimization |

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Simon Charette):

The docs are correct for `save` but primary keys are still updatable
through `queryset.filter(pk=old_value).update(pk=new_value)`.

Feel free to submit a PR. It'd be great to investigate if other backends
have a similar option as well.

--
Ticket URL: <https://code.djangoproject.com/ticket/30375#comment:4>

Django

unread,

Apr 30, 2019, 3:08:00 PM4/30/19

to django-...@googlegroups.com

#30375: Use "NO KEY" when doing select_for_update for PostgreSQL
-------------------------------------+-------------------------------------

Reporter: Manuel Weitzman | Owner: nobody
Type: | Status: new
Cleanup/optimization |

Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:

Keywords: postgres, lock, | Triage Stage: Accepted
database, operation |
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Carlton Gibson):

* stage: Unreviewed => Accepted

--
Ticket URL: <https://code.djangoproject.com/ticket/30375#comment:5>

Django

unread,

May 1, 2019, 8:51:46 AM5/1/19

to django-...@googlegroups.com

#30375: Use "NO KEY" when doing select_for_update for PostgreSQL
-------------------------------------+-------------------------------------

Reporter: Manuel Weitzman | Owner: Manuel
Type: | Weitzman
Cleanup/optimization | Status: assigned

Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by felixxm):

* status: new => assigned
* needs_better_patch: 0 => 1
* owner: nobody => Manuel Weitzman
* needs_docs: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/30375#comment:6>

Django

unread,

May 11, 2020, 10:55:02 AM5/11/20

to django-...@googlegroups.com

#30375: Use "NO KEY" when doing select_for_update for PostgreSQL
-------------------------------------+-------------------------------------

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Mads Jensen):

* needs_better_patch: 1 => 0
* needs_docs: 1 => 0

Comment:

This needs to be in the review queue :)

--
Ticket URL: <https://code.djangoproject.com/ticket/30375#comment:7>

Django

unread,

May 21, 2020, 1:34:43 AM5/21/20

to django-...@googlegroups.com

#30375: Use "NO KEY" when doing select_for_update for PostgreSQL
-------------------------------------+-------------------------------------

Keywords: postgres, lock, | Triage Stage: Ready for
database, operation | checkin
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by felixxm):

* stage: Accepted => Ready for checkin

--
Ticket URL: <https://code.djangoproject.com/ticket/30375#comment:8>

Django

unread,

May 21, 2020, 5:38:55 AM5/21/20

to django-...@googlegroups.com

#30375: Use "NO KEY" when doing select_for_update for PostgreSQL
-------------------------------------+-------------------------------------

Reporter: Manuel Weitzman | Owner: Manuel
Type: | Weitzman

Cleanup/optimization | Status: closed

Component: Database layer | Version: master
(models, ORM) |

Severity: Normal | Resolution: fixed

Keywords: postgres, lock, | Triage Stage: Ready for
database, operation | checkin

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Mariusz Felisiak <felisiak.mariusz@…>):

* status: assigned => closed
* resolution: => fixed

Comment:

In [changeset:"a4e6030904df63b3f10aa0729b86dc6942b0458e" a4e6030]:
{{{
#!CommitTicketReference repository=""
revision="a4e6030904df63b3f10aa0729b86dc6942b0458e"
Fixed #30375 -- Added FOR NO KEY UPDATE support to
QuerySet.select_for_update() on PostgreSQL.
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/30375#comment:9>

Reply all

Reply to author

Forward

[Django] #30375: Use "NO KEY" when doing select_for_update for PostgreSQL

Django

Django

Django

Django

Django

Django

Django

Django

Django

Django