Opened 7 weeks ago

Closed 7 weeks 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 weeks ago.
Screen of strange select stack trace

Download all attachments as: .zip

Change History (5)

Changed 7 weeks ago by M1ha Shvn

Attachment: screen.png added

Screen of strange select stack trace

comment:1 Changed 7 weeks ago by Tim Graham

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

comment:2 in reply to:  1 ; Changed 7 weeks ago by 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

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

comment:3 in reply to:  2 Changed 7 weeks ago by M1ha Shvn

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 weeks ago by M1ha Shvn (previous) (diff)

comment:4 Changed 7 weeks ago by M1ha Shvn

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