Fixed #31685 -- Added support for updating conflicts to QuerySet.bulk_create(). #13065

ChihSeanHsu · 2020-06-14T17:34:29Z

Adding upsert_conflicts arguments to bulk_create.
This change only supports to mysql, sqlite3 and postgresql.

unittest
refactor
document

apollo13 · 2020-06-16T15:48:53Z

I just gave the patch a quick glance, but I am wondering if it wouldn't make more sense to have the argument similar to the ForeignKey.on_delete, ie on_conflict = models.IGNORE | models.UPDATE? This might allow for more advanced "on conflict update set xyz=aaa" forms in the future.

ChihSeanHsu · 2020-06-16T16:17:15Z

I just gave the patch a quick glance, but I am wondering if it wouldn't make more sense to have the argument similar to the ForeignKey.on_delete, ie on_conflict = models.IGNORE | models.UPDATE? This might allow for more advanced "on conflict update set xyz=aaa" forms in the future.

It's a good idea. I'll change this argument to what you suggest.
Thanks for your help.

felixxm · 2020-06-16T17:13:08Z

I just gave the patch a quick glance, but I am wondering if it wouldn't make more sense to have the argument similar to the ForeignKey.on_delete, ie on_conflict = models.IGNORE | models.UPDATE? This might allow for more advanced "on conflict update set xyz=aaa" forms in the future.

It was discussed in the original ticket-28668 and #9192, see comment.

felixxm · 2020-06-16T17:14:32Z

I would change upsert_conflicts to update_conflicts (True/False).

ChihSeanHsu · 2020-06-18T14:47:10Z

I would change upsert_conflicts to update_conflicts (True/False).

Okay, I've changed upsert to update.
And you provide the topic that is about not using string constant instead of boolean argument.
I use boolean arguments(update_conflicts, ignore_conflicts) for high-level function bulk_create , but use string constant in low-level function that's called by bulk_create. Because we can decrease some duplicate code by using string constant in low-level function.

I don' know it is a good way to doing like this or not.
Could anyone give some advice?

chrisconlan · 2020-10-27T15:04:43Z

Thank you for writing this pull request. Looking forward to using this feature

jacobtylerwalls · 2020-10-28T17:44:42Z

Hi @ChihSeanHsu I noticed you ticked Needs Tests and Needs Documentation on the ticket, which signals to reviewers this isn't ready to review. Be sure to untick those if you've got tests and docs and you're ready for a review. Also this would be a good time to resolve the merge conflict. Thanks!

ChihSeanHsu · 2020-11-01T14:35:36Z

Hi @ChihSeanHsu I noticed you ticked Needs Tests and Needs Documentation on the ticket, which signals to reviewers this isn't ready to review. Be sure to untick those if you've got tests and docs and you're ready for a review. Also this would be a good time to resolve the merge conflict. Thanks!

Hi @jacobtylerwalls,
Thanks for your notification, I'll merge conflict later.

docs/ref/models/querysets.txt

alextatarinov · 2020-11-09T10:45:50Z

Hi @ChihSeanHsu, great job! However, I think there is some room for improvement.
With ignore_conflicts it makes sense to just say "ON CONFLICT DO NOTHING" (on any conflict), but IMHO it is more complicated with upsert.
There is more than fields with unique=True that can produce the conflict. At least, there are unique_together, and actually, there can be any unique constraint added either via RunSQL or manual database change, and we don't know what exactly is going to conflict. Therefore, there is a need for a more granular API, which allows users to specify both unique_fields for the ON CONFLICT part and update_fields, similarly to update_or_create.
We are using this library, and in my experience, it does not seem possible to automatically infer the intended behavior (what to update on what fields conflict) and must be specified by the developer.

hannseman · 2020-11-09T11:09:36Z

@alextatarinov good point, we would need similar logic to what's used in Model._get_unique_checks to take unique_together and UniqueConstraint in account:

django/django/db/models/base.py

Lines 1037 to 1045 in 10f8b82

    
           unique_togethers = [(self.__class__, self._meta.unique_together)] 
        
           constraints = [(self.__class__, self._meta.total_unique_constraints)] 
        
           for parent_class in self._meta.get_parent_list(): 
        
               if parent_class._meta.unique_together: 
        
                   unique_togethers.append((parent_class, parent_class._meta.unique_together)) 
        
               if parent_class._meta.total_unique_constraints: 
        
                   constraints.append( 
        
                       (parent_class, parent_class._meta.total_unique_constraints) 
        
                   )

But as you say it would probably make more sense to let the user specify the fields to conflict on.

ChihSeanHsu · 2020-11-09T14:54:52Z

Hi @alextatarinov , thanks for your suggestion, I'll add it to this pull request.

docs/ref/models/querysets.txt

ChihSeanHsu · 2020-11-15T16:46:44Z

Hi @alextatarinov and @hannseman,

I have a question need to clarify, this new feature is based on bulk_create and it is based on INSERT ... ON DUPLICATE KEY DO ... or INSERT ... ON CONFLICT (...) DO ....
Conflicts will only happen when pk or unique keys duplicates, because it's a insert action.
IMHO, we only need to let user input what fields(update_fields) they want to update, and I'll add more detailed in get unique_fields.
Or do you two have any other concern?

And upsert function in this library is not using this SQL command, it controls this action in application level not database.

alextatarinov · 2020-11-15T17:56:56Z

Hello again, @ChihSeanHsu. If you specify native=True, the linked library uses the ON CONFLICT statement via the querybuilder library.
I see where your logic goes, but I think there is a reason why the conflict target is required to provide when using DO UPDATE (https://www.postgresql.org/docs/9.5/sql-insert.html#SQL-ON-CONFLICT). It means the database cannot automatically infer the conflict target, neither we can do it in Django. I am not sure why, one thing that comes to mind is that the row inserted can conflict with multiple other rows on different constraints, and the database won't know which row to update. Therefore, it's the developer who should specify unique fields that may lead to conflict + update.
On a side note, I think the inability to specify unique_fields for ignore_conflicts is also a shortcoming and should be addressed (maybe even in this ticket).

P.S. I am nowhere an expert, so I suggest we ask someone more experienced.
P.P.S. It turns out, that when multiple rows conflict, all of them are updated in MySQL (which does not allow specifying unique_fields). You can also add a LIMIT to update only X first rows matched. https://stackoverflow.com/a/16377933.
Now I really wonder why PostgreSQL requires the conflict_target with ON UPDATE. I believe PostgreSQL is better designed than MySQL in general, so I would say their approach is preferable (require to specify unique_fields). But if I am not mistaking, this is not supported by MySQL, unfortunately. Not sure which API and behavior would be the best then.

hannseman · 2020-11-15T19:50:29Z

django/db/backends/base/operations.py

+
+        for unique_together in opts.unique_together:
+            unique_fields.extend(unique_together)
+


You should also collect class based unique-constraints, you get can them from opts.total_unique_constraints.

hannseman · 2020-11-15T20:05:53Z

I'm not really sure that collecting all unique fields and then using them for conflict_target will work in PostgresSQL. My understanding is that it expects a field or a set of fields that can be inferred as a unique constraint. In other words the fields that is passed as conflict_target needs to match a single unique constraint. https://www.postgresql.org/docs/current/sql-insert.html#SQL-ON-CONFLICT

For example:

CREATE TABLE person(
    email varchar UNIQUE,
    phone varchar UNIQUE,
    name varchar
);

persons = [Person(name="Some name", phone="+46700000000", email="test@example.com")]
Person.objects.bulk_create(persons, update_conflicts=True, update_fields=["name"])

Would generate the following SQL:

INSERT INTO person(email, phone, name) VALUES ('test@example.com', '+46700000000', 'Some name')
ON CONFLICT (email, phone)
DO UPDATE SET name = EXCLUDED.name;

This would error with ERROR: there is no unique or exclusion constraint matching the ON CONFLICT specification as no composite index on (email, phone) exists.

ChihSeanHsu · 2020-11-20T16:56:34Z

Hello again,

Hello again, @ChihSeanHsu. If you specify native=True, the linked library uses the ON CONFLICT statement via the querybuilder library.
I see where your logic goes, but I think there is a reason why the conflict target is required to provide when using DO UPDATE (https://www.postgresql.org/docs/9.5/sql-insert.html#SQL-ON-CONFLICT). It means the database cannot automatically infer the conflict target, neither we can do it in Django. I am not sure why, one thing that comes to mind is that the row inserted can conflict with multiple other rows on different constraints, and the database won't know which row to update. Therefore, it's the developer who should specify unique fields that may lead to conflict + update.
On a side note, I think the inability to specify unique_fields for ignore_conflicts is also a shortcoming and should be addressed (maybe even in this ticket).

P.S. I am nowhere an expert, so I suggest we ask someone more experienced.
P.P.S. It turns out, that when multiple rows conflict, all of them are updated in MySQL (which does not allow specifying unique_fields). You can also add a LIMIT to update only X first rows matched. https://stackoverflow.com/a/16377933.
Now I really wonder why PostgreSQL requires the conflict_target with ON UPDATE. I believe PostgreSQL is better designed than MySQL in general, so I would say their approach is preferable (require to specify unique_fields). But if I am not mistaking, this is not supported by MySQL, unfortunately. Not sure which API and behavior would be the best then.

After I surveyed and as you mentioned, the unique_fields argument is only for user using postgresql and sqlite.
If user use mysql as their database, they only can specify update_fields.
In my opinion, it's not a good solution, but there is this kind of limit.

I'm not really sure that collecting all unique fields and then using them for conflict_target will work in PostgresSQL. My understanding is that it expects a field or a set of fields that can be inferred as a unique constraint. In other words the fields that is passed as conflict_target needs to match a single unique constraint. https://www.postgresql.org/docs/current/sql-insert.html#SQL-ON-CONFLICT

For example:
CREATE TABLE person(
    email varchar UNIQUE,
    phone varchar UNIQUE,
    name varchar
);
persons = [Person(name="Some name", phone="+46700000000", email="test@example.com")]
Person.objects.bulk_create(persons, update_conflicts=True, update_fields=["name"])
Would generate the following SQL:
INSERT INTO person(email, phone, name) VALUES ('test@example.com', '+46700000000', 'Some name')
ON CONFLICT (email, phone)
DO UPDATE SET name = EXCLUDED.name;
This would error with ERROR: there is no unique or exclusion constraint matching the ON CONFLICT specification as no composite index on (email, phone) exists.

And I will add more unittest to cover this situation.

lithammer · 2020-12-21T17:00:09Z

Not sure if it's my place to comment but... I wonder if the API could be made a bit more flexible by modeling a bit more like QuerySet.update()? For example we have a use-case where we want increment fields when there's a conflict, rather than replacing. Something similar to this:

CREATE TABLE foo (
    timestamp timestamp NOT NULL,
    num_requests integer NOT NULL
);

INSERT INTO
    foo (timestamp, num_requests)
VALUES
    ('2020-12-21T12:00:00', 10)
ON CONFLICT
    (timestamp)
DO UPDATE SET
    num_requests = foo.num_requests + EXCLUDE.num_requests;

foos = [
    Foo(timestamp="2020-12-21T12:00:00", num_requests=F("num_requests") + 10),
]
Foo.objects.bulk_create(foos, update_conflicts=True, update_fields=["num_requests"])

Not sure how portable that is though, or if it's mostly a PostgreSQL thing.

ChihSeanHsu · 2021-01-24T14:41:21Z

"num_requests"

Hi @lithammer,

It's a good suggestion, but bulk_create action is base on InsertQuery and F is not allowed to use in it.
If we want to support this flexible usage, we need to refactor lot of code.

But if there is requirement, we can discuss it.

jacobtylerwalls · 2021-10-17T18:58:55Z

Thanks for the continued work here. Is this ready for re-review? If so, please make sure to uncheck Needs Improvement on the ticket each time it's ready for re-review. (Would be helpful if you could also bump the release note and change annotations to 4.1). Cheers.

ChihSeanHsu · 2021-10-18T14:34:06Z

Thanks for the continued work here. Is this ready for re-review? If so, please make sure to uncheck Needs Improvement on the ticket each time it's ready for re-review. (Would be helpful if you could also bump the release note and change annotations to 4.1). Cheers.

Hi @jacobtylerwalls ,
Thanks for your remind.
I've update release note to 4.1 and change ticket status.

felixxm

@ChihSeanHsu Thanks for updates 👍 I left more comment, however didn't go through docs and tests in detail. I'll do this in the next round.

Please add also release notes for the new feature.

docs/ref/models/querysets.txt

django/db/models/query.py

docs/ref/models/querysets.txt

django/db/backends/mysql/operations.py

tests/bulk_create/tests.py

django/db/backends/base/operations.py

django/db/backends/sqlite3/features.py

django/db/models/query.py

tests/bulk_create/tests.py

felixxm

@ChihSeanHsu Thanks 👍 I left more comment. Again I didn't go through docs and tests in detail.

django/db/backends/sqlite3/operations.py

django/db/models/query.py

django/db/backends/sqlite3/operations.py

tests/bulk_create/tests.py

django/db/models/query.py

docs/ref/models/querysets.txt

felixxm

@ChihSeanHsu Thanks for updates and all your efforts 👍 I left small comments.

It's almost ready for my final review/edits 🥇

django/db/models/query.py

tests/bulk_create/tests.py

docs/ref/models/querysets.txt

felixxm · 2022-01-17T06:14:53Z

@ChihSeanHsu Do you have time to keep working on this?

ChihSeanHsu · 2022-01-17T15:22:16Z

@ChihSeanHsu Do you have time to keep working on this?

@felixxm
Sorry.
I was a little busy previous months and did not notice that it was under another reviews.
I'll update it later, thanks.

felixxm · 2022-01-18T06:12:37Z

I squashed commits and rebased from the main branch. I'll work on final edits.

felixxm

@ChihSeanHsu I pushed edits to docs and main changes. I'm going to review the tests tomorrow.

django/db/backends/mysql/operations.py

django/db/models/query.py

felixxm · 2022-01-19T09:28:18Z

@ChihSeanHsu Thanks again for all your efforts 🥇 Welcome aboard ⛵

I pushed edits to tests. IMO, it's ready 🥳

…_create(). Thanks Florian Apolloner, Chris Jerdonek, Hannes Ljungberg, Nick Pope, and Mariusz Felisiak for reviews.

ChihSeanHsu · 2022-01-20T14:50:50Z

@ChihSeanHsu Thanks again for all your efforts 🥇 Welcome aboard ⛵

I pushed edits to tests. IMO, it's ready 🥳

@felixxm
Thanks for your helps and I'm glad to do this ticket.

ChihSeanHsu marked this pull request as draft June 14, 2020 17:34

ChihSeanHsu marked this pull request as ready for review June 16, 2020 16:06

ChihSeanHsu force-pushed the dev/bulk_create_allow_upsert branch from af2f8ff to 0f92932 Compare June 18, 2020 14:47

ChihSeanHsu force-pushed the dev/bulk_create_allow_upsert branch from 342a490 to b42781d Compare November 1, 2020 14:34

hannseman reviewed Nov 5, 2020

View reviewed changes

docs/ref/models/querysets.txt Outdated Show resolved Hide resolved

hannseman reviewed Nov 9, 2020

View reviewed changes

docs/ref/models/querysets.txt Outdated Show resolved Hide resolved

hannseman reviewed Nov 15, 2020

View reviewed changes

ChihSeanHsu marked this pull request as draft November 22, 2020 17:40

ChihSeanHsu force-pushed the dev/bulk_create_allow_upsert branch 2 times, most recently from d554d68 to fe3e848 Compare December 6, 2020 16:25

ChihSeanHsu force-pushed the dev/bulk_create_allow_upsert branch from fe3e848 to d8a6c77 Compare January 24, 2021 13:16

ChihSeanHsu marked this pull request as ready for review January 24, 2021 14:41

Base automatically changed from master to main March 9, 2021 06:21

ChihSeanHsu force-pushed the dev/bulk_create_allow_upsert branch 4 times, most recently from ddd5ab3 to 0da99dd Compare October 18, 2021 14:05

felixxm reviewed Oct 22, 2021

View reviewed changes

felixxm reviewed Oct 25, 2021

View reviewed changes

felixxm reviewed Nov 17, 2021

View reviewed changes

adamchainz mentioned this pull request Jan 6, 2022

Support INSERT ... ON DUPLICATE KEY UPDATE ... adamchainz/django-mysql#501

Closed

felixxm self-assigned this Jan 18, 2022

felixxm force-pushed the dev/bulk_create_allow_upsert branch 2 times, most recently from 637bd4f to 54244ac Compare January 18, 2022 06:12

felixxm force-pushed the dev/bulk_create_allow_upsert branch 2 times, most recently from 5f2e849 to c32e98f Compare January 18, 2022 11:06

felixxm reviewed Jan 18, 2022

View reviewed changes

django/db/backends/mysql/operations.py Show resolved Hide resolved

django/db/models/query.py Outdated Show resolved Hide resolved

django/db/models/query.py Outdated Show resolved Hide resolved

felixxm force-pushed the dev/bulk_create_allow_upsert branch 2 times, most recently from f055eaa to 9aeee21 Compare January 19, 2022 09:27

felixxm force-pushed the dev/bulk_create_allow_upsert branch from 9aeee21 to fba23da Compare January 19, 2022 09:51

felixxm changed the title ~~#31685 -- Support updating conflicts with QuerySet.bulk_create().~~ Fixed #31685 -- Added support for updating conflicts to QuerySet.bulk_create(). Jan 19, 2022

felixxm force-pushed the dev/bulk_create_allow_upsert branch from fba23da to 0e62ffe Compare January 19, 2022 10:01

Fixed #31685 -- Added support for updating conflicts to QuerySet.bulk…

0f69464

…_create(). Thanks Florian Apolloner, Chris Jerdonek, Hannes Ljungberg, Nick Pope, and Mariusz Felisiak for reviews.

felixxm force-pushed the dev/bulk_create_allow_upsert branch from 0e62ffe to 0f69464 Compare January 19, 2022 19:19

felixxm merged commit 0f69464 into django:main Jan 20, 2022

HamaBarhamou mentioned this pull request Dec 11, 2023

Added #34277 -- Add Conditional WHERE Clause to bulk_create for SQLite and PostgreSQL #17515

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed #31685 -- Added support for updating conflicts to QuerySet.bulk_create(). #13065

Fixed #31685 -- Added support for updating conflicts to QuerySet.bulk_create(). #13065

ChihSeanHsu commented Jun 14, 2020 •

edited

apollo13 commented Jun 16, 2020 •

edited

ChihSeanHsu commented Jun 16, 2020

felixxm commented Jun 16, 2020

felixxm commented Jun 16, 2020

ChihSeanHsu commented Jun 18, 2020

chrisconlan commented Oct 27, 2020

jacobtylerwalls commented Oct 28, 2020 •

edited

ChihSeanHsu commented Nov 1, 2020

alextatarinov commented Nov 9, 2020 •

edited

hannseman commented Nov 9, 2020

ChihSeanHsu commented Nov 9, 2020

ChihSeanHsu commented Nov 15, 2020

alextatarinov commented Nov 15, 2020 •

edited

hannseman Nov 15, 2020 •

edited

hannseman commented Nov 15, 2020 •

edited

ChihSeanHsu commented Nov 20, 2020

lithammer commented Dec 21, 2020 •

edited

ChihSeanHsu commented Jan 24, 2021

jacobtylerwalls commented Oct 17, 2021

ChihSeanHsu commented Oct 18, 2021

felixxm left a comment

felixxm left a comment

felixxm left a comment

felixxm commented Jan 17, 2022

ChihSeanHsu commented Jan 17, 2022

felixxm commented Jan 18, 2022

felixxm left a comment

felixxm commented Jan 19, 2022

ChihSeanHsu commented Jan 20, 2022


		for unique_together in opts.unique_together:
		unique_fields.extend(unique_together)

Fixed #31685 -- Added support for updating conflicts to QuerySet.bulk_create(). #13065

Fixed #31685 -- Added support for updating conflicts to QuerySet.bulk_create(). #13065

Conversation

ChihSeanHsu commented Jun 14, 2020 • edited

apollo13 commented Jun 16, 2020 • edited

ChihSeanHsu commented Jun 16, 2020

felixxm commented Jun 16, 2020

felixxm commented Jun 16, 2020

ChihSeanHsu commented Jun 18, 2020

chrisconlan commented Oct 27, 2020

jacobtylerwalls commented Oct 28, 2020 • edited

ChihSeanHsu commented Nov 1, 2020

alextatarinov commented Nov 9, 2020 • edited

hannseman commented Nov 9, 2020

ChihSeanHsu commented Nov 9, 2020

ChihSeanHsu commented Nov 15, 2020

alextatarinov commented Nov 15, 2020 • edited

hannseman Nov 15, 2020 • edited

Choose a reason for hiding this comment

hannseman commented Nov 15, 2020 • edited

ChihSeanHsu commented Nov 20, 2020

lithammer commented Dec 21, 2020 • edited

ChihSeanHsu commented Jan 24, 2021

jacobtylerwalls commented Oct 17, 2021

ChihSeanHsu commented Oct 18, 2021

felixxm left a comment

Choose a reason for hiding this comment

felixxm left a comment

Choose a reason for hiding this comment

felixxm left a comment

Choose a reason for hiding this comment

felixxm commented Jan 17, 2022

ChihSeanHsu commented Jan 17, 2022

felixxm commented Jan 18, 2022

felixxm left a comment

Choose a reason for hiding this comment

felixxm commented Jan 19, 2022

ChihSeanHsu commented Jan 20, 2022

ChihSeanHsu commented Jun 14, 2020 •

edited

apollo13 commented Jun 16, 2020 •

edited

jacobtylerwalls commented Oct 28, 2020 •

edited

alextatarinov commented Nov 9, 2020 •

edited

alextatarinov commented Nov 15, 2020 •

edited

hannseman Nov 15, 2020 •

edited

hannseman commented Nov 15, 2020 •

edited

lithammer commented Dec 21, 2020 •

edited