Opened 7 years ago

Closed 7 years ago

#28676 closed Bug (invalid)

Using select_for_update with next save() in multiple threads stucks

Reported by: M1ha Shvn Owned by: nobody
Component: Database layer (models, ORM) Version: 1.10
Severity: Normal Keywords: django psycopg PostgreSQL select_for_update
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I use django 1.10.7, PostgreSQL 9.4, psycopg 2.7.3.1 with gunicorn server.
One of celery tasks can be executed simmultiniously by multiple workers.
Here is the code:

    with transaction.atomic(using=settings.DB_SHARD):
        # if I remove select_for_update here, everything works fine
        # Second thread will wait here with "SELECT ... FOR UPDATE" query in database with "active" status
        conv = Conversation.objects.select_for_update().get(pk=116774)

        # Some code, changing conv object without sending
        conv.user_unread_count = 0

        # First thread is stuck here
        # I see "UPDATE ..." query in database with "idle in transaction" status
        conv.save(update_fields=['user_unread_count'])

        # This doesn't work as well
        # conv.save(update_fields=['user_unread_count'], force_update=True)

When 2 workers start on the same Conversation object, second thread is waiting on select_for_update (which is well), and the first is stuck in save() operation.

I've ran this code with django_debug toolbar and got stange select, done by save() method:

SELECT ••• FROM "messages_conversation" WHERE "messages_conversation"."id" = 116774 FOR UPDATE
-- ??? What is this select here for
SELECT ••• FROM "messages_conversation" WHERE "messages_conversation"."id" = 116774 

UPDATE "messages_conversation" SET "user_unread_count" = 0 WHERE "messages_conversation"."id" = 116774

Moreover, I've fixed the bug by changing save() with update(), it doesn't do strange select:

    with transaction.atomic(using=settings.DB_SHARD):
        conv = Conversation.objects.select_for_update().get(pk=116774)

       # Some code here
  
        Conversation.objects.filter(id=conv.id).update(user_unread_count=0)

Attachments (1)

screen.png (148.0 KB ) - added by M1ha Shvn 7 years ago.
Screen of strange select stack trace

Download all attachments as: .zip

Change History (5)

by M1ha Shvn, 7 years ago

Attachment: screen.png added

Screen of strange select stack trace

comment:1 by Tim Graham, 7 years ago

Can you explain why Django is at fault and propose a change to fix it?

in reply to:  1 ; comment:2 by M1ha Shvn, 7 years ago

Replying to Tim Graham:

Can you explain why Django is at fault?

1) According to PostgreSQL docs state "idle in transaction" means that query was executed and control returned to backend code, but it hasn't committed the transaction yet. But save() is stuck somewhere (it is incorrect). If I place print(123) after it - it will not print anything. So PostgreSQL ended its work, but save() method stopped, waiting for something and not continuing code execution. The expected behavior of django here is to leave save() method, than leave "with transaction.atomic" context manager and commit transaction, so other transaction can go in. The proof is replacing save() method with QuerySet.update(). As you can see on the screen, it generates perfectly the same SQL, but works fine without stucking.
2) The second strange factor is duplicate select query, generated inside save() method. As you can see from above and screen, it's the same query without "FOR UPDATE". But there is no need in this query - conversation data has been already selected by "SELECT ... FOR UPDATE". According to the docs, save() should do only one query - INSERT or UPDATE (UPDATE in this situation) without any SELECT queries.

  1. s. The only reason for save of doing second SELECT from the docs is getting pk value. But it is not INSERT query, pk is already defined. Moreover force_update=True doesn't change anything.

Replying to Tim Graham:

propose a change to fix it

I'm not quite good in django inner code, I've tried debugging where, inside save() method, the problem is, but haven't succeeded

Version 1, edited 7 years ago by M1ha Shvn (previous) (next) (diff)

in reply to:  2 comment:3 by M1ha Shvn, 7 years ago

Replying to M1ha Shvn:

Replying to Tim Graham:

Can you explain why Django is at fault?

1) According to PostgreSQL docs state "idle in transaction" means that query was executed and control returned to backend code, but it hasn't committed the transaction yet. But save() is stuck somewhere (it is incorrect). If I place print(123) after it - it will not print anything. So PostgreSQL ended its work, but save() method stopped, waiting for something and not continuing code execution. The expected behavior of django here is to leave save() method, than leave "with transaction.atomic" context manager and commit transaction, so other transaction can go in. The proof is replacing save() method with QuerySet.update(). As you can see on the screen, it generates perfectly the same SQL, but works fine without stucking.
2) The second strange factor is duplicate select query, generated inside save() method. As you can see from above and screen, it's the same query without "FOR UPDATE". But there is no need in this query - conversation data has been already selected by "SELECT ... FOR UPDATE". According to the docs, save() should do only one query - INSERT or UPDATE (UPDATE in this situation) without any SELECT queries.

  1. s. The only reason for save() of doing second SELECT from the docs is getting pk value. But it is not INSERT query, pk is already defined. Moreover force_update=True doesn't change anything.

Replying to Tim Graham:

propose a change to fix it

I'm not quite good in django inner code, I've tried debugging where, inside save() method, the problem is, but haven't succeeded

Futher investigation shows that django-cacheops invalidated_update() also doesn't work. Seems it's bug of this extension.
Here is the related bug.

Last edited 7 years ago by M1ha Shvn (previous) (diff)

comment:4 by M1ha Shvn, 7 years ago

Resolution: invalid
Status: newclosed
Note: See TracTickets for help on using tickets.
Back to Top