v3 #2
v3 #2
Conversation
Implement the spirit of previous versions in a single file with just a few simple functions. Adds inherent support for prepared statements, as well as QueryRowContext. It is very plausible that this module, which is now less than 100 lines of code, is a good candidate for copy-paste installation.
|
This is awesome! I like how succinct it is. Are you planning on utilizing prepared statements more moving forward? |
sqldb.go
Outdated
| if err != nil { | ||
| return 0, err // already normalized | ||
| } | ||
| count += rows |
There was a problem hiding this comment.
Is it helpful to sum the count of affected rows if multiple statements are being executed? Would it be more helpful to return a slice of counts or count of successful statement executions?
There was a problem hiding this comment.
Great point/questions. The big advantages of this v3 proposal are:
- it is at least an order of magnitude more succinct,
- it doesn't completely bury the standard library API (like v2 does), and
- it preserves the spirit of the current (v2) API in order to ease potential migrations, should we decide to go this direction.
There was a problem hiding this comment.
Running multiple statements in a single call is clunky by design, IMO. I'm sure we could come up with a better API for this, but that wasn't my top priority.
There was a problem hiding this comment.
That's fair. I like the succinctness of v3 and I think we should move this direction. I am just a little concerned that this is going to be confusing. You're right that it's inherently an issue and there isn't a perfect solution so I don't have an issue punting this.
There was a problem hiding this comment.
The need to split multiple statements into multiple execution statements is actually a limitation of the MySQL driver specifically. Other drivers (pgsql) can handle multiple statements. The split with the subsequent aggregation of rows affected is masking the fact that it's multiple, separate statements being executed.
The count as a single integer is important for optimistic concurrency, e.g. 27 rows in total were affected and we expected 30 rows affected therefore something must have changed underneath us.
|
@kierstenSmarty - I don't really have immediate plans, but perhaps we should be considering this for some of our more active paths. |
..lest we pollute the main module's dependency listing. This suite utilizes an in-memory sqlite database to avoid any need for a running database server.
This DB leverages prepared statements when configured thresholds are encountered. The DB type is the approach I'd like to use for new code. Existing mappers can be more easily transitioned to the package-level functions that were implemented in previous commits on this v3 branch. Perhaps those functions will be moved to a separate package...
No more package-level/static functions.
I'm allowing these contractual interfaces to be exported so they can be conveniently referenced by calling code. This decision was made after careful consideration of intended implementation patterns as I actually coded against the new API, which helped determine that the ability to reference these type definitions provides significant value.
The sqldb module has had a very 'enterprise' feel to it, but it's not really doing that much. This PR proposes an implementation that retains the behavior of previous versions but does so with less than 100 lines of code. It also adds inherent support for prepared statements, as
well as
QueryRowContext.See this commit for the new implementation at a glance: 7295811