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

Transaction support #424

Merged
merged 16 commits into from
May 19, 2021
Merged

Transaction support #424

merged 16 commits into from
May 19, 2021

Conversation

zachmu
Copy link
Member

@zachmu zachmu commented May 19, 2021

This change:

  • Moves a bunch of transaction and other session management code out of Handler into Engine
  • Introduces a new set of tests around transactions
  • Fixes the error message for duplicate key violations
  • Removes AsyncNode and related code
  • Eliminates duplicate query parsing

Copy link
Contributor

@andy-wm-arthur andy-wm-arthur left a comment

Choose a reason for hiding this comment

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

LGTM!

engine.go Outdated Show resolved Hide resolved
engine.go Show resolved Hide resolved
enginetest/testdata.go Show resolved Hide resolved
// This is a bit odd, but because this setup doesn't interact with the engine.Query path, we need to do transaction
// management here, instead. If we don't, then any Query-based setup will wipe out our work by starting a new
// transaction without committing the work done so far.
// The secondary foo database doesn't have this problem because we don't mix and match query and non-query setup
Copy link
Contributor

Choose a reason for hiding this comment

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

I've wondered if we shouldn't refactor testdata to just query the harness

Copy link
Member Author

Choose a reason for hiding this comment

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

The issue is that then we couldn't run the engine tests on read only database implementations, so it's worth some amount of futzing to support

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually now that I'm saying that, it's already not true, they have to implement sql.InsertableTable at a minimum already

@zachmu zachmu merged commit 937632d into master May 19, 2021
@Hydrocharged Hydrocharged deleted the zachmu/tx branch December 8, 2021 07:07
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