fix: Remove unnecessary transaction

[mitar]

While resolving an issue in my own implementation of storage for Fosite which has been modeled after Hydra's, I realized:

• There is unnecessary transaction in this function. The first query (deleting expired assertions) is completely unrelated from the other one (inserting a new assertion). In fact, deletion could even be done as a background job and not inside this function (in fact, why it is not a background job, or in a goroutine).
• Transaction isolation level used here is a default one for the connection (i.e., read committed) while BeginTX uses LevelRepeatableRead. Is this on purpose or is this a bug? In my implementation I always used the same way to create a transaction so I always had repeatable read transactions, which combined with the unnecessary transaction in this function lead to commit conflicts (because multiple parallel transactions were attempting to delete same expired assertions).

Checklist

• I have read the contributing guidelines.
• I have referenced an issue containing the design document if my change
  introduces a new feature.
• I am following the
  contributing code guidelines.
• I have read the security policy.
• I confirm that this pull request does not address a security
  vulnerability. If this pull request addresses a security. vulnerability, I
  confirm that I got green light (please contact
  security@ory.sh) from the maintainers to push
  the changes.
• I have added tests that prove my fix is effective or that my feature
  works.
• I have added or changed the documentation.

[codecov]

Codecov Report

Merging #3029 (f2aadd5) into master (3115dde) will decrease coverage by 0.02%.
The diff coverage is 63.63%.

❗ Current head f2aadd5 differs from pull request most recent head 51bd9c8. Consider uploading reports for the commit 51bd9c8 to get more accurate results

@@            Coverage Diff             @@
##           master    #3029      +/-   ##
==========================================
- Coverage   79.42%   79.40%   -0.03%     
==========================================
  Files         112      112              
  Lines        7889     7889              
==========================================
- Hits         6266     6264       -2     
- Misses       1222     1223       +1     
- Partials      401      402       +1     

Impacted Files	Coverage Δ
persistence/sql/persister_oauth2.go	81.01% <63.63%> (-0.85%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fff6491...51bd9c8. Read the comment docs.

[aeneasr]

You're right, that should indeed be a background task. But we can merge this as is already, thank you!

[mitar]

Note: are you sure it is OK that some transactions are not using LevelRepeatableRead? So you have to code paths here to create transactions and one has LevelRepeatableRead level and the other does not. Is this on purpose?

[aeneasr]

Since RepeatableRead is the highest isolation level, and might only manifest at significant scale, I think it's fine for now to not set them explicitly. Some databases like CRDB have RR as the default level in any case :)

[mitar]

Sorry to nitpick, but "serializable" is the highest isolation level. :-)

Also, we had issues (and this is how I discovered this code) at a pretty low scale: it was pretty common to get "transaction aborted" with repeatable read before this code change, because the delete query touches the whole table, so two parallel queries had always a conflict.

Anyway, if you know what you are doing and why is LevelRepeatableRead even set there in the first place, then it is OK. :-)

[aeneasr]

I see, to be honest I haven't looked into the code in a while and it always requires cognitive load to do that. If this is an issue that is bothering you in production and is causing problems in delivering the service, please feel free to open an issue with a problem statement, reproducible case, and ideas on how to resolve it. If that's not the case I'll refrain from trying to think myself through this :)

[mitar]

No, issues on my side has been resolved. So this is all fine now. I just wanted to put this isolation-level discrepancy on your radar so that if in the future you might learn about some issue with isolation, you might remember that parts of queries are using one isolation level and parts another.

[aeneasr]

Love it, thanks :) You show a high level of trust in my ability to remember discussions. I hope it's not misplaced 😅

[mitar]

With some luck, I might also notice it and pitch in. :-)

Have a good night!

[aeneasr]

Thank you, you too!