[Django] #28514: Documentating idempotence of Many2ManyField.add()
Django
------------------------------------------------+------------------------
Reporter: Дилян Палаузов | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: 1.11
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------------+------------------------
At https://docs.djangoproject.com/en/1.11/topics/db/examples/many_to_many/
is written:
>> a2.publications.add(p3)
>
> Adding a second time is OK:
>
>> a2.publications.add(p3)
Please rephrase it to "Adding a second time is OK, it doesn't duplicate
the relation".
At https://docs.djangoproject.com/en/1.11/ref/models/relations/ is
written:
add(*objs, bulk=True)
Adds the specified model objects to the related object set.
Please amend: "Inserting existing relations this way does not cause
problems."
The purpose of the changes is to make clear, that add() is idempotent and
it is not necessary first to read the data from the database, join with
the new values and eventually write the resulting set back.
--
Ticket URL: <https://code.djangoproject.com/ticket/28514>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
--------------------------------------+------------------------------------
Reporter: Дилян Палаузов | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: 1.11
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
--------------------------------------+------------------------------------
Changes (by Tim Graham):
* easy: 0 => 1
* stage: Unreviewed => Accepted
Old description:
> At
> https://docs.djangoproject.com/en/1.11/topics/db/examples/many_to_many/
> is written:
>
> >> a2.publications.add(p3)
> >
> > Adding a second time is OK:
> >
> >> a2.publications.add(p3)
>
> Please rephrase it to "Adding a second time is OK, it doesn't duplicate
> the relation".
>
> At https://docs.djangoproject.com/en/1.11/ref/models/relations/ is
> written:
>
> add(*objs, bulk=True)
> Adds the specified model objects to the related object set.
>
> Please amend: "Inserting existing relations this way does not cause
> problems."
>
> The purpose of the changes is to make clear, that add() is idempotent and
> it is not necessary first to read the data from the database, join with
> the new values and eventually write the resulting set back.
New description:
At https://docs.djangoproject.com/en/1.11/topics/db/examples/many_to_many/
it's written:
{{{
>> a2.publications.add(p3)
>
> Adding a second time is OK:
>
>> a2.publications.add(p3)
}}}
Please rephrase it to "Adding a second time is OK, it doesn't duplicate
the relation".
At https://docs.djangoproject.com/en/1.11/ref/models/relations/ it's
written:
{{{
add(*objs, bulk=True)
Adds the specified model objects to the related object set.
}}}
Please amend: "Inserting existing relations this way does not cause
problems."
The purpose of the changes is to make clear, that `add()` is idempotent
and it is not necessary first to read the data from the database, join
with the new values and eventually write the resulting set back.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/28514#comment:1>
Django
Reporter: Дилян Палаузов | Owner: Jack
Type: | Mustacato
Cleanup/optimization | Status: assigned
Component: Documentation | Version: 1.11
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Changes (by Jack Mustacato):
* owner: nobody => Jack Mustacato
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/28514#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: Дилян Палаузов | Owner: Jack
Type: | Mustacato
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Changes (by Jack Mustacato):
* has_patch: 0 => 1
Comment:
Made suggested changes: https://github.com/django/django/pull/8953
--
Ticket URL: <https://code.djangoproject.com/ticket/28514#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: Дилян Палаузов | Owner: Jack
Type: | Mustacato
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
Comment:
I've reviewed the PR, it all looks good to me.
--
Ticket URL: <https://code.djangoproject.com/ticket/28514#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: Дилян Палаузов | Owner: Jack
Type: | Mustacato
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
* stage: Ready for checkin => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/28514#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Дилян Палаузов | Owner: Jack
Type: | Mustacato
Cleanup/optimization | Status: assigned
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
Reporter: Дилян Палаузов | Owner: (none)
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: 1.11
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 1 | UI/UX: 0
Changes (by Jack Mustacato):
* owner: Jack Mustacato => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/28514#comment:7>
Django
Reporter: Дилян Палаузов | Owner: Levi
Type: | Payne
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
Changes (by Levi Payne):
* status: new => assigned
* owner: (none) => Levi Payne
--
Ticket URL: <https://code.djangoproject.com/ticket/28514#comment:8>
Django
Reporter: Дилян Палаузов | Owner: (none)
Component: Documentation | Version: 1.11
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 1 | UI/UX: 0
Changes (by Levi Payne):
* status: assigned => new
* owner: Levi Payne => (none)
--
Ticket URL: <https://code.djangoproject.com/ticket/28514#comment:9>
Django
--------------------------------------+------------------------------------
Reporter: Дилян Палаузов | Owner: (none)
Component: Documentation | Version: 1.11
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 Jezeniel Zapanta):
I could work on this one, this is my first time though. Could I assign
this to me?
--
Ticket URL: <https://code.djangoproject.com/ticket/28514#comment:10>
Django
Reporter: Дилян Палаузов | Owner: Jezeniel
Type: | Zapanta
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
Changes (by Jezeniel Zapanta):
* status: new => assigned
* owner: (none) => Jezeniel Zapanta
--
Ticket URL: <https://code.djangoproject.com/ticket/28514#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: Дилян Палаузов | Owner: Jezeniel
Type: | Zapanta
Cleanup/optimization | Status: assigned
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 Jezeniel Zapanta):
Hi, submitted the PR, for your review.
--
Ticket URL: <https://code.djangoproject.com/ticket/28514#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: Дилян Палаузов | Owner: Jezeniel
Type: | Zapanta
Cleanup/optimization | Status: assigned
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 Дилян Палаузов):
Regarding the PR at https://github.com/django/django/pull/9314 , as noted
in my last comment above, .add() works not as documented, if two threats
call it with partially overlapping set of objects.
--
Ticket URL: <https://code.djangoproject.com/ticket/28514#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: Дилян Палаузов | Owner: Jezeniel
Type: | Zapanta
Cleanup/optimization | Status: assigned
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 Jezeniel Zapanta):
Replying to [comment:13 Дилян Палаузов]:
> Regarding the PR at https://github.com/django/django/pull/9314 , as
noted in my last comment above, .add() doesn't work as documented, if two
threads call it with partially overlapping set of objects.
What is the next step? Should I change the documentation that to handle
concurrency problems you should handle IntegrityError? or wait for
[https://code.djangoproject.com/ticket/28668 #28668] to be Implemented?
--
Ticket URL: <https://code.djangoproject.com/ticket/28514#comment:14>
Django
-------------------------------------+-------------------------------------
Reporter: Дилян Палаузов | Owner: Jezeniel
Type: | Zapanta
Cleanup/optimization | Status: assigned
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 Дилян Палаузов):
I don't think that the ON CONFLICT DO NOTHING change will get into Django
2.0, even if #28668 was fixed yesterday.
Under these circumstances for the next year or so {{{.add(one more
elements)}}} can raise an {{{IntegrityError}}} or send wrong m2m_changed
signals and this should be documented.
--
Ticket URL: <https://code.djangoproject.com/ticket/28514#comment:15>
Django
-------------------------------------+-------------------------------------
Reporter: Дилян Палаузов | Owner: Jezeniel
Type: | Zapanta
Cleanup/optimization | Status: assigned
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 Дилян Палаузов):
Any progress here?
I think only the concurrent aspect of .add() is missing:
* if two .add() are called in parallel with overlapping set of related
objects, one of them can fail with IntegrityError and has to be repeated.
This is caused by the fact that Django inserts the objects within a
transaction and only after the transaction is closed it can be determined
if the changes are permament. Django does not retry in this case
* When IntegrityError is thrown, m2m_changed is still sent for the
objects, that were supposed to be matched.
The latter is valid if I recall correctly the circumstances from five
months ago. The code can certainly be changed so that m2m_changed is not
sent, but it is better to document the current behaviour than doing
noting.
Jack, I would like to encourage you to use the opportunity and check the
implementation, while you describe it.
Apart from this my plan is to work with Django for one more month and I
will be glad if by then all concerns related to this ticket can be
clarified.
--
Ticket URL: <https://code.djangoproject.com/ticket/28514#comment:16>
Django
-------------------------------------+-------------------------------------
Reporter: Дилян Палаузов | Owner: Jezeniel
Type: | Zapanta
Cleanup/optimization | Status: assigned
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 Tim Graham):
If `RelatedManager.add()` isn't idempotent, then that sounds like a bug.
We prefer to spend time fixing bugs rather than documenting them, so
that's the approach I'd suggest.
--
Ticket URL: <https://code.djangoproject.com/ticket/28514#comment:17>
Django
-------------------------------------+-------------------------------------
Reporter: Дилян Палаузов | Owner: Jezeniel
Type: | Zapanta
Cleanup/optimization | Status: assigned
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 Дилян Палаузов):
This approach is nice, but status quo is misleading the ones who try to
use {{{.add()}}}.
If currently nobody has the time to fix the bug, then it is more likely
that somebody will have the time to document the behaviour, as this
requires less time.
--
Ticket URL: <https://code.djangoproject.com/ticket/28514#comment:18>
Django
-------------------------------------+-------------------------------------
Reporter: Дилян Палаузов | Owner: Jezeniel
Type: | Zapanta
Cleanup/optimization | Status: assigned
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 Дилян Палаузов):
Undoubtedly https://github.com/django/django/pull/9314 shall be
integrated, it fixes the problem this ticket initially raised and is
independent of other problems .add() has.
Unfortunately on the above link is shown some tests have failed, but I
cannot see now why.
The concurrent mystery is tracked under #19544.
--
Ticket URL: <https://code.djangoproject.com/ticket/28514#comment:19>
Django
-------------------------------------+-------------------------------------
Reporter: Дилян Палаузов | Owner: Jezeniel
Type: | Zapanta
Component: Documentation | Version: 1.11
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"abe6c5defefc7057e7fb5f47b79643f7b89f7d90" abe6c5de]:
{{{
#!CommitTicketReference repository=""
revision="abe6c5defefc7057e7fb5f47b79643f7b89f7d90"
Fixed #28514 -- Clarifed docs about idempotence of RelatedManager.add().
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28514#comment:20>
Django
-------------------------------------+-------------------------------------
Reporter: Дилян Палаузов | Owner: Jezeniel
Type: | Zapanta
Cleanup/optimization | Status: closed
Severity: Normal | Resolution: fixed
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 Tim Graham <timograham@…>):
In [changeset:"83362cb94a730c1c88752313e2b1b10ed5f436c0" 83362cb]:
{{{
#!CommitTicketReference repository=""
revision="83362cb94a730c1c88752313e2b1b10ed5f436c0"
[2.0.x] Fixed #28514 -- Clarifed docs about idempotence of
RelatedManager.add().
Backport of abe6c5defefc7057e7fb5f47b79643f7b89f7d90 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28514#comment:21>