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

Add TxOptions to txn.Begin #3327

Merged
merged 1 commit into from
Sep 27, 2022
Merged

Add TxOptions to txn.Begin #3327

merged 1 commit into from
Sep 27, 2022

Conversation

dnephin
Copy link
Contributor

@dnephin dnephin commented Sep 26, 2022

This will allow us to start read only transactions, or transactions with different isolation levels.

We're not using that yet, but after #3302 merges we'll be able to use it for handlers. An example of that can be seen here #3328.

We will also need this for long-polling. We need to use an isolation level that ensures that multiple queries in the same transaction all read the same snapshot (repeatable read).

// TODO: accept sql.TxOptions when we remove gorm
func (d *DB) Begin(ctx context.Context) (*Transaction, error) {
tx := d.DB.WithContext(ctx).Begin()
func (d *DB) Begin(ctx context.Context, opts *sql.TxOptions) (*Transaction, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could consider using variadic arguments to allow the opts to be optional.

Suggested change
func (d *DB) Begin(ctx context.Context, opts *sql.TxOptions) (*Transaction, error) {
func (d *DB) Begin(ctx context.Context, opts ...sql.TxOptions) (*Transaction, error) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see gorm uses this pattern, but I consider this dangerous and I don't think it should ever be used.

varargs are great if you are going to use all the args, but using varargs for a single optional argument and ignoring the rest is going to cause confusion for anyone reading the code. Adding a nil for the parameter isn't really a big inconvenience. We could also follow the stdlib and have two functions for Begin, but at this time it didn't seem necessary since it was easy enough to add the nils.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it's just a suggestion. The nil argument is a bit ugly, especially when that's the default case. From the project I've seen, this is a really common pattern and familiar to many Go devs.

@dnephin dnephin changed the base branch from main to dnephin/authorize-using-request-context September 27, 2022 15:55
@dnephin dnephin force-pushed the dnephin/sql-tx-options branch 2 times, most recently from 80bb5f3 to fe66be6 Compare September 27, 2022 16:02
@dnephin dnephin force-pushed the dnephin/authorize-using-request-context branch from 28d353f to c63f4c4 Compare September 27, 2022 16:44
@dnephin dnephin force-pushed the dnephin/authorize-using-request-context branch from c63f4c4 to 8420901 Compare September 27, 2022 17:59
This will allow us to start read only transactions, or transactions with
different isolation levels.
Base automatically changed from dnephin/authorize-using-request-context to main September 27, 2022 18:59
@dnephin dnephin merged commit 27804f6 into main Sep 27, 2022
@dnephin dnephin deleted the dnephin/sql-tx-options branch September 27, 2022 19:02
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