move transaction.atomic contexts to lower level #5790
Conversation
| @@ -495,8 +494,7 @@ def _create_object_from_params(self, lookup, params): | |||
| Used by get_or_create and update_or_create | |||
| """ | |||
| try: | |||
| with transaction.atomic(using=self.db): | |||
| obj = self.create(**params) | |||
| obj = self.create(**params) | |||
| return obj, True | |||
| except IntegrityError: | |||
There was a problem hiding this comment.
This makes makes it not obvious at all that the contents of the try/except IntegrityError are properly wrapped in a transaction.
This is necessary to prevent errors on databases which actually enforce transactional integrity on errors like PostgreSQL.
|
It's unclear to me that this change preserves transactional integrity of changes in cases of concrete inheritance. |
|
My understanding was that save or create calls will anyway end up calling _insert _update or update down the stack. So moving atomic context there should make execution of queries go more smoothly by avoiding wrapping extra code into transaction.atomic. Does it make sense for you? |
|
What if a single call to |
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.
|
The case you've mentioned is taken care by this code in Model.save_base: |
|
I don't think this PR is correct at all. transaction.atomic() is removed from a couple of places where save() or create() is called, and then transaction.atomic() is added to all _insert/_create operations. Also, the claim that this increases parallelism and performance isn't backed up by any data, and I don't see any obvious reason why this would be so. |
|
Fair enough. Closing this PR. will send another one. |
Wrapping 'execute_sql' into transaction.atomic instead of
wrapping 'create' and 'save' should increase parallelism and
spead up execution of 'insert' and 'update' SQL queries.