sql,pg,engine: rework sql interfaces #601
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jchappelow
commented
Mar 13, 2024
Comment on lines
-215
to
-227
| dbVoting, err := dbtest.NewTestDB(t) | ||
| txVoting, _ := dbVoting.BeginTx(ctx) | ||
| err = voting.InitializeVoteStore(ctx, txVoting) | ||
| db, cleanup, err := dbtest.NewTestPool(ctx, []string{schemaName, "kwild_voting"}) // db is the event store specific connection | ||
| require.NoError(t, err) | ||
| _, err = txVoting.Precommit(ctx) | ||
| require.NoError(t, err) | ||
| err = txVoting.Commit(ctx) | ||
| require.NoError(t, err) | ||
| // defer // drop kwild_voting | ||
| defer func() { | ||
| dbVoting.AutoCommit(true) | ||
| dbVoting.Execute(ctx, `drop schema if exists kwild_voting cascade;`) | ||
| }() |
There was a problem hiding this comment.
This 13 line monstrosity is where I started looking for the issues with the pg and sql interfaces and concrete types.
brennanjl
approved these changes
Mar 15, 2024
|
lgtm, will let you merge since the first line says DO NOT MERGE |
Reduce sql interfaces, and clean up pg methods.
jchappelow
force-pushed
the
pg-clean-test
branch
from
69ff718 to
5af4d4a
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reduce sql interfaces, and clean up pg methods.
This change does:
rework all of the interfaces in
common/sql/sql.gofor clarity and versatility. See updated type docs there. Types and interfaces are generally defined from lowest to highest level of composition for easy of understanding.With a
*pg.DB, it was previously required to callPrecommitbeforeCommit, but made the type less able to effectively satisfy the simpler sql interfaces, and it was technically of no need if a commit ID (or 2pc) was not required. Now it is optional, simplifying it's use in contexts where these features are not relevant (only commit or rollback needed).Remove
AccessModefrom theDB(and thusTx) interfaces, making it an optional type assertion required by the engine for mutable actions. This wasn't totally necessary to change, but it was an awkward method for all other systems and their tests.Remove many obsolete types and methods from
internal/sql/pg/conn.go, mostly from when we had implicit sessions rather than transaction-scoped Executors.The
OuterTxMakerinterface no longer collides withTxMaker, allowing*pg.DBto satisfy both.Fix
(*Pool).BeginReadTxbeginning a RW txn instead of a RO txn.Globally narrow the interfaces on function input arguments to what is actually required. This improves semantics and testability.
Many other less notable changes...
Some significant ugliness in the event store test setup (events_tests.go) is eliminated.