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

database/sql: add API and give it some love #16673

Closed
kardianos opened this issue Aug 11, 2016 · 28 comments
Closed

database/sql: add API and give it some love #16673

kardianos opened this issue Aug 11, 2016 · 28 comments
Assignees
Milestone

Comments

@kardianos
Copy link
Contributor

@kardianos kardianos commented Aug 11, 2016

The database/sql API works well in many cases and the simplicity is extremely appealing. However is has been noted that if done again it wouldn't be in the std lib (maybe in an "x" repo).

Here are some of the major issues with database/sql:

  • No context support / difficult to add in now (easy to leak connections). #15123
  • No multiple result set support (sql server, postgresql, my sql, oracle use this). #12382
  • Unable to specify a transaction level (this is important for certain cases).
  • No support for named input parameters (mature database tables can get wide, counting columns has huge potential to introduce bugs and reduces clarity). #12381
  • No support for custom types #13567 #16235

Here are some of the minor issues with database/sql:

  • Unable to get result database types. #16652
  • Unable to get a single connection from the pool (needed for out of transaction queries but retain a temp tables that are specific to a session).
  • Unable to set parameter database types (can be important, can result in type mismatch issues). #12383
  • No API for bulk insert operations (rdbms often have an alternate API for this). #5171
  • No Savepoint / Rollback To API (often suggested in place of nested transactions: sql server, postgresql, my sql). #7898
  • No support for output parameters (often used esp when a shop requires all logic in stored procs / functions).
  • Developers often want type string from database types varchar and text, useful to be able to have as a default (with option to get as bytes to reduce allocs if desired).
  • Cannot stream out large values (many databases support 1GB values, want to stream them out in an io.Writer). (possible tie in with #13567 )

Some of these may have been omitted by design, some by oversight, and some because the API was not available yet (context). However it is possible to make a database/sql compatible API from a more powerful API (I've done this). In this case I'm strongly favor making a more powerful API available. Due to fundamental issues with adding a context to the existing API, I propose freezing the database/sql package and moving development to an "golang.org/x" repo.


I would want to survey the developer community before proceeding. I also have a proposed API I am developing https://godoc.org/github.com/kardianos/rdb which is an evolution of an API I have in production today.

I would be interested in hearing feedback from the go team (cc @bradfitz ) and others in the community who connect to rdbms (cc @kostya-sh @nightlyone @realpg @joegrasse @asambeka @kirubasankars )

@quentinmit
Copy link
Contributor

@quentinmit quentinmit commented Aug 11, 2016

+1, I think it makes sense to move to x/ and add context, at least.

@tgulacsi
Copy link
Contributor

@tgulacsi tgulacsi commented Aug 11, 2016

+1, I'd love to see an API which allowed out parameters (pointers) inQuery/Exec. Now I have to use the db specific library directly.

@dlsniper
Copy link
Contributor

@dlsniper dlsniper commented Aug 11, 2016

I'd like to add "no tracer support" to the list of missing features.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Aug 11, 2016

I don't think any of these feature requests are insurmountable, and I don't think moving focus to x/ will make developers magically appear. If there are people who want to hack on this but don't feel like they can because it's in the standard library, please come forward. You have my blessing to hack on any of the open issues:

https://github.com/golang/go/issues?utf8=%E2%9C%93&q=is%3Aissue%20is%3Aopen%20in%3Atitle%20database

I think moving to x/ will only split the community, back to how it was before database/sql was in the standard library.

But I agree it's probably time to give database/sql some more love. With the Go 1.8 tree open, we have three months. Now's the time for everybody to start hacking.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Aug 11, 2016

I'd like to add "no tracer support" to the list of big nopes.

I'm not sure how to read that. Can you clarify without a double negative?

@dlsniper
Copy link
Contributor

@dlsniper dlsniper commented Aug 11, 2016

Updated thr comment. I will try and contribute it in 1.8

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Aug 11, 2016

I'd like to add "no tracer support" to the list of missing features.

By tracer do you mean golang.org/x/net/trace? A dependency of that would be context support, right?

@kardianos
Copy link
Contributor Author

@kardianos kardianos commented Aug 11, 2016

@bradfitz I certainly wouldn't mind contributing to the package. I just don't see how the API can be compatible to many of these issue I've encountered. If you are willing to take that list given above, and maybe give an idea of how to implement it w/o breaking API, I'd help flush out the API and send in code.

If the community had an initial split, and then moved over eventually, I'd be fine with that. Esp if a compatibility layer was added.

@derekperkins
Copy link

@derekperkins derekperkins commented Aug 11, 2016

I agree it's probably time to give database/sql some more love. With the Go 1.8 tree open, we have three months. Now's the time for everybody to start hacking.

Context support is a must.

How often are people jumping between databases? That seems like the main reason to have the database/sql abstraction in the first place. I've been curious what the reaction has been to https://github.com/jackc/pgx. That seems like an interesting way to handle driver/db specific functionality.

@derekperkins
Copy link

@derekperkins derekperkins commented Aug 11, 2016

@kardianos For some reason your link to godoc is causing a 503.

@kardianos
Copy link
Contributor Author

@kardianos kardianos commented Aug 11, 2016

(godoc.org seems to be having trouble in general right now)
I should add, right now features missing mean often today often mean people don't use database/sql, they use pgx or the driver directly. I use the v1 of the listed rdb package to access ms sql server for a line of business application, would have been much harder w/ database/sql.

But again, if the API can fit the changes, I'd help add them to the existing package.

@asambeka
Copy link

@asambeka asambeka commented Aug 11, 2016

@kardianos Completely agree with you on this. I'd add connection options to the list.

@leeview
Copy link

@leeview leeview commented Aug 12, 2016

@kardianos I totally agree database/sql needs a redesign to include support for the features in your list (how was multiple result set support left out ?), but why not go the usual route: start the new project as golang.org/x/sql2 and promote it to standard lib only when it reaches a reasonable maturity ?

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Aug 12, 2016

@leeview, we won't move "v2" packages into the standard library to live alongside their v1 versions. Ever since Go 1, we've been moving away from the standard library towards using external repos. If sql2 happens outside the tree, it will stay outside the tree. But I'd rather improve the one we've got instead.

@kostya-sh
Copy link
Contributor

@kostya-sh kostya-sh commented Aug 12, 2016

I share Brad's concern that developers won't magically appear even if this proposal is accepted. At least some maintainers of the currently popular DB drivers should agree to work on this.

An incremental approach of improving database/sql might work better. I agree though that some of the missing functionality is hard to add without breaking at least database/sql/driver API. Maybe database/sql/driver2 can help with this?

@joegrasse
Copy link

@joegrasse joegrasse commented Aug 12, 2016

I also agree with what @bradfitz said about improving database/sql rather than it happening outside the tree.

@kardianos
Copy link
Contributor Author

@kardianos kardianos commented Aug 12, 2016

I'm willing to work on it either way. I had filled previous issues but I
didn't think they would be accepted so I took my work elsewhere. Perhaps I
misunderstood.

Brad, of the issues listed above, which ones could you see going in to the
SQL package?

EDIT: @bradfitz Or another way, are there any of the above points you couldn't see going into the sql package?

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Aug 12, 2016

@kardianos In general if an issue was not closed then it was accepted as something to at least consider fixing. But that is different from somebody actually working on it.

@kardianos
Copy link
Contributor Author

@kardianos kardianos commented Aug 12, 2016

@ianlancetaylor Ah. Then it was my mistake. Yeah, I can work on some of these this cycle.

@kardianos
Copy link
Contributor Author

@kardianos kardianos commented Aug 12, 2016

@bradfitz @kostya-sh
I'm willing to work on the existing sql package. I've tried to look over some of the existing API issues and offer possibilities, but I'd like feedback before I start coding.

After doing an overview, it certainly isn't impossible to continue the current API and add features. I will try to pursue adding to the API, but I think it is a mistake; we will be left with a split API and split driver support either way.

I've made a rough list of features that will probably cause a driver and API split, and those that won't:

Cause split:

  • Add context
  • Specify Transaction Isolation Level
  • Specify Query Isolation Level
  • Named input parameters
  • Get result database types
  • Set input parameter database types
  • Support output parameters
  • Switch to output go string type for database text, char, varchar, nvarchar, varchar2 types

Safely add:

  • Multiple result sets
  • Support custom types (Scan into array, map[string]interface{}, XML obj) [probably safe to add]
  • Get a single connection from the pool
  • Add bulk insert operation
  • Add Savepoint and RollbackTo
  • Support streaming large values (large binary values to prevent buffering everything) [probably safe to add]
@kardianos
Copy link
Contributor Author

@kardianos kardianos commented Aug 13, 2016

From conversations with on various issues I would categorize the above issues in a few categories:

Plan to add to database/sql{/driver}

  • Add Context #15123
  • Add API to read multiple result sets. #12382
  • Add Savepoint / Rollback To AP #7898
  • Other non-api connection pool bugs

May add to database/sql{/driver}

  • Named input parameters #12381

Probably won't go into database/sql{/driver}

  • Specify transaction level
  • Support for custom types per #13567 #16235
  • Get result database types #16652
  • Get a single connection from the pool
  • Set input parameter database type (option to cast to driver defined type) #12381 (comment)
  • Bulk insert API #5171
  • No output parameters, related to #12381
  • Do not default to go type string for database text/varchar types. No API to hang option off of.
  • Stream large values out with io.Writer (related to #13567 )

Let me know if you think any of these are miscategorized or preemptively categorized.

@andlabs
Copy link
Contributor

@andlabs andlabs commented Sep 2, 2016

What do you mean by "transaction level"?

@sjmudd
Copy link

@sjmudd sjmudd commented Sep 8, 2016

MySQL recently announced a new protocol, MySQL X protocol for talking to MySQL as of 5.7.12. See: https://dev.mysql.com/doc/internals/en/x-protocol.html. One of the current features it offers are "command pipelining" which current database/sql does not support. It's not clear from comments above if that sort of behaviour would be able to fit into the new driver being discussed here. Certainly database/sql as it stands is unable to support this and also has no hooks to poke into the lower level driver to make use of these features. I'm currently looking at writing a driver to support this protocol and am not sure how to expose as much of the functionality as possible via database/sql which would be more convenient as writing a completely new driver would require a much larger investment. So this issue seems to be the most attractive solution to follow.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Sep 8, 2016

@sjmudd, there's no reason a pipelined protocol wouldn't work with database/sql.

@kardianos kardianos changed the title proposal: officially freeze database/sql - possibly move development to x repo database/sql: add API and give it some love Sep 8, 2016
@dominikh
Copy link
Member

@dominikh dominikh commented Sep 12, 2016

By tracer do you mean golang.org/x/net/trace? A dependency of that would be context support, right?

Rather something like httptrace but for database/sql and database/sql/driver – for example to allow integrating with OpenTracing, and other tracing instrumentation that is not x/net/trace. And yes, it would require context support.

@sjmudd
Copy link

@sjmudd sjmudd commented Sep 26, 2016

@bradfitz: please clarify? What I had in mind is a way to send a query and be able to send another one prior to having received the result set of the first and doing this over the same connection.

As far as I can see use of db.Query() is currently synchronous, and therefore depends to some extent on the combination of network and query latency. Pipelining here helps if you can send the next query prior to having processed the results of the previous one.

@kardianos
Copy link
Contributor Author

@kardianos kardianos commented Nov 28, 2016

Closing this issue as the major issues have been addressed or are on the table to be addressed. I've filed a few additional issues to ensure they are tracked, or explicitly rejected.

@kardianos kardianos closed this Nov 28, 2016
@golang golang locked and limited conversation to collaborators Nov 28, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.