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

Prevent nested transactions with *DB.Transaction #3

Merged
merged 1 commit into from Jul 19, 2020

Conversation

qilx
Copy link

@qilx qilx commented Jul 11, 2020

Make sure these boxes checked before submitting your pull request.

  • Do only one thing
  • No API-breaking changes
  • New code/logic commented & tested

What did this pull request do?

This will prevent creating nested transactions while calling *DB.Transaction(...) multiple times.
Only fist call will create transaction. Rest will just reuse it.

@dynajoe
Copy link

dynajoe commented Jul 19, 2020

Could this be used in .Begin() for those that don't always use .Transaction?

@jinzhu jinzhu merged commit 345d18c into jinzhu:master Jul 19, 2020
@qilx
Copy link
Author

qilx commented Jul 19, 2020

It is probably not useful put it into .Begin(). Because Because even so you have to call .Commit() only once at the end.
Only solution that I can think of is putting also counter into *DB, which would count number of .Begin() calls and then ignored counter-1 calls to .Commit(). But I would say that it is easier to just use .Transaction() (and more error prone).

@sjmtan
Copy link

sjmtan commented Jul 24, 2020

I find this interface to be a bit confusing, but I don't use it - so take my feedback with a grain of salt. I think this changes the semantics of the function a bit - when you write the function block as a programmer, you sort of assume that at the end of the function block, it'll be committed since that's the behavior in most cases.

Only in the nested case is this not true if I'm reading it correctly, and you can imagine issues arising if the "nested" transaction has an RPC call after.

In my company's codebase, we panic on these nested transactions in tests/removing ways to have nested transactions, and while I'm not saying that it is the ideal behavior, it forces our developers to make sure that they are aware of what data is committed or not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants