Fixed #25939 -- Removed transaction from QuerySet.update_or_create's save().

[bartosh]

There is no need to wrap calls of create and save methods in
transaction.atomic as it's already done down the call stack in
Model.save_base method.

[bartosh]

This is a second PR. You can see first one here: #5790

[akaariai]

I think this one requires a savepoint, but create() calls save(), and save has savepoint=False.

[akaariai]

Also, the atomic(savepoint=False) is really cheap (no DB action at all) when used inside another atomic. So, the atomic() here makes the atomic() inside save cheap enough to not matter (of course, benchmarks tell the truth).

[bartosh]

Thanks, I'll update PR according to your comments.

Just wanted to say that I'm getting "OperationalError: database is locked" exception without first hunk applied. It happens on get_or_create call.
Can you suggest what can be the reason for that? I'm using sqlite if it matters.

[aaugustin]

If you're getting database is locked on SQLite, most likely you shouldn't be using SQLite in the first place. I don't expect the kind of changes you're proposing here to have a significant effect.

Please seek help:

• on the django-users mailing list to discuss what optimizations you could use in your project
• on the django-developers mailing list to discuss changes in Django; these changes will have to be backed up by benchmarks showing an improvement

[bartosh]

Thanks for your suggestion. I can only say that with this patch applied I can't reproduce the error. There should be a reason for that. Suggesting to stop using sqlite is a work around, not a solution from my point of view.

[bartosh]

Updated PR according to reviewer suggestions. Please, merge if you don't mind.

[aaugustin]

https://www.sqlite.org/faq.html#q5

[bartosh]

aaugustin, thank you for the info. I hope it will help me to find the reason for the issue.
Just checked out what you've pointed to:
ATOMIC_REQUESTS is not in use - true
update_or_create ends up creating an object - true
the save method of that objects does long things like sending email after the write (i.e. once it holds an exclusive lock on the database) - not true. save is not redefined in the model.

Just to be clear. The issue is gone when both hunks of original patch are applied. It's still there with moving save out of transaction.atomic context. I'll check if another hunk alone fixes it or not.

[bartosh]

aaugustin, I've read sqlite FAQ before. what's written there doesn't explain the fact that I'm getting dozens of 'database is locked' issues and they're gone if I move obj = self.create(**params) out of transaction.atomic context in query.py

Any other ideas where to look and what potentially could be the reason?

[aaugustin]

I have no idea about your use case, your code, or the kind of concurrency you're facing. If you could reproduce this with a minimal example and a simulate load, then we could benchmark it.

[bartosh]

yep, makes sense. I'll try to sketch some example. meanwhile feel free to merge this commit. I'll create new PR when I get the data.

Thank you,
Ed

[timgraham]

Could you add a mention in the 1.10 release notes "Backwards-incompatible" changes section as requested by Aymeric? "This may affect query counts tested by assertNumQueries in TransactionTestCase which should be mentioned in the release notes."

[bartosh]

reopen as it was closed by mistake.

[bartosh]

Updated release notes as suggested.

[timgraham]

merged in 423b3af, thanks!