Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid DB savepoints if no audit is going to be created #232

Merged
merged 1 commit into from
Sep 4, 2022

Conversation

hugobranquinho
Copy link
Contributor

@hugobranquinho hugobranquinho commented Jun 6, 2022

If we save a model with a db transaction and that model don't have audit active, it's making unnecessary db calls:

SAVEPOINT "xpto_x1";
RELEASE SAVEPOINT "xpto_x1";

This happens because django creates a save point if a transaction is open inside another transaction.
To avoid it, and save some db calls, do should_audit(instance) before creating transaction.atomic.

If we save a model with a db transaction active and that model don't have audit active, it's making unnecessary db calls:
```
SAVEPOINT "xpto_x1";
RELEASE SAVEPOINT "xpto_x1";
```

This happens because django creates a save point if a transaction is open inside another transaction.
To avoid it, and save some db calls, do `should_audit(instance)` before creating `transaction.atomic`.
@jheld
Copy link
Collaborator

jheld commented Jun 30, 2022

So I do agree that if we can avoid certain paths in the flow, we should, and I'm still concurring that this is a good change, but is it actually hitting the database in the situations where the audit isn't going to be performed? Or is it more that we're looking to avoid needless extra (or any at all) db save points in the current transaction?

@hugobranquinho
Copy link
Contributor Author

@jheld
Yup, is hitting the db to create and release the save point (2 db queries)
Because on my project we don't have audit for all tables (i think we have more without audit) every change we do on those, we have 2 extra queries. For example, if we make 50 object updates and none of those have audit, we have 150 db calls.

@jheld jheld merged commit c78920b into soynatan:master Sep 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants