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

feat: add tx methods to IDB (#587) #591

Merged
merged 1 commit into from
Jul 2, 2022

Conversation

isgj
Copy link
Contributor

@isgj isgj commented Jun 26, 2022

Updated IDB interface with 2 methods

BeginTx(ctx context.Context, opts *sql.TxOptions) (Tx, error)
RunInTx(ctx context.Context, opts *sql.TxOptions, f func(ctx context.Context, tx Tx) error) error

When creating a savepoint the opts are ignored.
Currently the transaction will not use the name but we can store something like TX_{random} same format as for savepoint. Then we can also emit the name with the hooks so logging libraries can show if a query is being executed as part of a transaction or savepoint.

db.go Outdated

// BeginTx will save a point in the running transaction.
func (tx Tx) BeginTx(ctx context.Context, _ *sql.TxOptions) (Tx, error) {
uid, err := uuid.NewRandom()
Copy link
Member

Choose a reason for hiding this comment

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

Let's not add a dependency on uuid just to generate a random string.

    b := make([]byte, 16)
    rand.Read(b)

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 used crypto/rand but I think also math/rand is an option as it is safe for concurrent usage (for this usage pseudo random is fine). Maybe we can check which is faster but that will be one word change.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, crypto/rand is fine I guess.

@vmihailenco
Copy link
Member

This looks pretty good. Could you add a test for savepoints in https://github.com/uptrace/bun/blob/master/internal/dbtest/db_test.go#L273?

@isgj isgj force-pushed the feat/add-tx-idb branch 2 times, most recently from 2332441 to 46eb321 Compare June 28, 2022 17:12
@isgj
Copy link
Contributor Author

isgj commented Jun 28, 2022

mssql has a different syntax https://docs.microsoft.com/en-us/sql/t-sql/language-elements/save-transaction-transact-sql?view=sql-server-ver16

How are these cases handled when different sql should be generated?

@vmihailenco
Copy link
Member

How are these cases handled when different sql should be generated?

You can add a feature flag for mssql here and use it like this

@isgj isgj force-pushed the feat/add-tx-idb branch 2 times, most recently from 0d2e3c1 to a29a1b4 Compare July 1, 2022 13:55
@isgj isgj force-pushed the feat/add-tx-idb branch from a29a1b4 to feab313 Compare July 1, 2022 15:44
@isgj isgj marked this pull request as ready for review July 1, 2022 15:53
@vmihailenco
Copy link
Member

Excellent, thank you 👍

@vmihailenco vmihailenco merged commit 46b475f into uptrace:master Jul 2, 2022
@isgj isgj deleted the feat/add-tx-idb branch July 2, 2022 11:00
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