#28514: Clarify docs regarding idempotence of RelatedManager.add()
-------------------------------------+-------------------------------------
     Reporter:  Дилян Палаузов       |                    Owner:  Jack
         Type:                       |  Mustacato
  Cleanup/optimization               |                   Status:  assigned
    Component:  Documentation        |                  Version:  1.11
     Severity:  Normal               |               Resolution:
     Keywords:                       |             Triage Stage:  Accepted
    Has patch:  1                    |      Needs documentation:  0
  Needs tests:  0                    |  Patch needs improvement:  1
Easy pickings:  1                    |                    UI/UX:  0
-------------------------------------+-------------------------------------

Comment (by Дилян Палаузов):

 It turns out, that add() suffers from concurrency problem and is therefore
 not idempotent:

 
django/db/models/fields/related_descriptors.py:create_forward_many_to_many_manager.ManyRelatedManager.add()
 calls self.add(), which does
 {{{
 with transaction.atomic(using=db. savepoint=False):
     self.._add_items(self.source_Field_name, self.target_field_name,
 *objs)
 }}}

 and _add_items does
 {{{
 with transaction.atomic(using=db, savepoint=False):
     signals.m2m_changes.send(...)
     self.through._default_manager.using(db).bulk_create([...])
 }}}

 Minor question: providing that add() is the only caller of _add_items,
 does the second transaction(savepoint=False) have added value?
 Major question: bulk_create([]) can fail, if two threads simultaneously
 try to insert the same data, in which case the m2m_changed signal is sent,
 but bulk_create fails completely.

 My proposal:
 * First try bulk_insert, if it does not throw exception, send m2m_changed
 signal.
 * Either update the documentation of .add() accordingly (if it throws
 IntegrityError, the caller shall retry the operation), or make .add() do
 the retries internally.
 * Once bulk_create(... on_conflict='ignore') [#28668] is implemented,
 revert the previous step, pass on_conflict='ignore' to bulk_create, and:
  * In case of Postgresql retrieve information from bulk_create which
 objects were actually inserted, and send only for them m2m_changed
  * For other databases, either send signal for all objects, even those
 which were in the database

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

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to [email protected].
To post to this group, send email to [email protected].
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/072.b79f90f648da067b3503166146b668c7%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

Reply via email to