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 support to explicitly set input parameter types and result column types #12383

Closed
kardianos opened this issue Aug 28, 2015 · 12 comments
Milestone

Comments

@kardianos
Copy link
Contributor

@kardianos kardianos commented Aug 28, 2015

Right now database/sql infers input parameter types and omits database column level types,

This can be hard to support do to each database vendor supporting different types or different variants of types, however I believe a practical solution exists.

There are two parts to this (1) support setting input parameter types and (2) fetching result column types.

  1. Input parameter values cannot always be correctly inferred and often databases will complain if types don't align with existing table types.

For example Oracle has both "varchar(N), varchar2(N), and char(N) data types. In greenfield work you will probably work with and use varchar2(N) type. Legacy systems may have any of those. For each one of those setting the length of the parameter can be, though is not always, meaningful.

In MS SQL Server there are several different time types as well as text types: You may need a Time, Date, DateTime, DateTime2, or DateTimeOffset. For text you may need a text, ntext, char(N), varchar(N), or nvarchar(N) type. Some of the data types, such as "ntext" vs "nvarchar(N)" or DateTime2 vs DateTimeOffset are important and impossible to auto-discover.

  1. The need for knowing exactly what column type a return value is is less pronounced, but present, especially with the default behavior where strings are returned as []byte. When creating a framework that supports ad-hoc queries it is often important to keep at least the "generic" datatype with the column, even when no results are returned. I've felt this need when creating reporting modules and when doing automatic marshalling where there isn't a direct representation. For instance I marshal (Timestap,DateTime2) different then (Timestampz, DateTimeOffset) when the transport is JSON. And the difference is important for the client (it get's interpreted differently).

...

For types I recommend a two tier approach: have a set of common types defined: sql.Text, sql.Timestamp, sql.Timestampz, sql.Int, sql.Decimal, sql.Float, but allow drivers to also register types like: or.Varchar2, ms.Nvarchar, pg.Array.

The driver then documents how the sql generic types map onto the database types. Most often the generic types can be used (or simply inferred), but when the a specific type is needed it is only a quick parameter set away. I have implemented something like this and I am pleased with the result.

@ianlancetaylor ianlancetaylor added this to the Unplanned milestone Aug 29, 2015
@kostya-sh
Copy link
Contributor

@kostya-sh kostya-sh commented Aug 29, 2015

Do you have any examples of 1)? IMHO a driver should care of converting to the right database representation. Could it be a bug in the driver?

Maybe the solution for this is to provide a standard interface for querying table metadata (list of columns with their types)?

Should database specific types be provided by the driver package? Not every database has all the types and sometimes they are different between databases (or even different versions of the same database).

@kardianos
Copy link
Contributor Author

@kardianos kardianos commented Aug 29, 2015

@kostya-sh I address the issue of types in this description. I even have an implementation that shows this works.

I don't want table meta-data, nor do I wish for separate API to get that meta-data. If you're talking CRUD queries, that might be fine. I'm talking about larger queries that do business logic and the queries types aren't necessarily represented in any table anywhere.

@kostya-sh
Copy link
Contributor

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

In case of PostgreSQL it seems that specifying types for parameters is redundant as the database itself provides this information to the dirver (based on the source of github.com/lib/pq).

@kardianos
Copy link
Contributor Author

@kardianos kardianos commented Aug 13, 2016

@kostya-sh You are correct. Prepared statements infer types and send those back to the client. As such you probably wouldn't utilize this in that case. Also postgresql text is compatible with varchar, unlike other systems where it maters. Now I shall get all my clients and client's vendors to use postgresql and we'll be set...

@kostya-sh
Copy link
Contributor

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

@kardianos, I wasn't suggesting to switch all your clients to PostgreSQL ;) My intention was to collect use cases that are not possible to implement correctly withput specifying parameter types.

After a bit more digging I found that there are cases when PostgreSQL cannot infer parameter types. For example the following SQL cannot be currently executed using database/sql:

select date_trunc('hour', $1)

This is because there are two date_trunc functions in PostgreSQL and without type information it is impossible to choose the function.

@noblehng
Copy link

@noblehng noblehng commented Sep 13, 2016

I think is better leave input parameter types to database drivers as they vary among different database venders.

We could also create a database/sql/commontypes package, but that could well be a third-party package. The only benefit I can see, that a commontypes package over driver specific, is that for code portability across different database. But that will depends on how your database drivers convert between database types and common types, and that might suit your needs. A better approach will be write a migrator that traversal the AST and substitute between driver types as your needs.

The problem here is that we can't really pass custom types to Exec() and Query() now. In database/sql, all args are converted to database/sql/driver.Value then pass to the underlying database driver, and Value only allow some primitive types. What the default converter does is auto-dereference to those values, and even we implement the Valuer interface with our custom type, it will rejected by IsValue() check in database/sql. The only way to by pass this check is use a prepared statement and implement ColumnConverter interface and the type should not implement Valuer.

The solution is let the custom types in database drivers implement the Valuer interface by return its value as is. Then let IsValue() accept those types by defined a new interface in database/sql/drivers, that IsValue() can call to check arguments' type of Exec() and Query() is acceptable by underlying drivers. And all the custom types should also implement this interface.

For output types, if needed, it could implement the Scanner interface. But I think it will be much rare than input types as converting database types to Go types is much clear than converting Go types to database types that could have length and precision requirements.

@noblehng
Copy link

@noblehng noblehng commented Sep 13, 2016

The interface added in database/sql/driver can be something like:

type InputValue interface {
    Validate() bool
}

Then IsValue() should call v.Validate() to validate custom types. And all custom types that can be a input parameter type should implement this interface and Valuer. This should works for both driver specific types and common types.

@kardianos
Copy link
Contributor Author

@kardianos kardianos commented Oct 11, 2016

@bradfitz Could you write why the driver.IsValue exists and specifically the value those calls add in the sql package convert.go driverArgs function? It looks like both of the calls in convert.go are on the driver.ColumnConverter path and as such it could be the driver's responsibility to check driver.IsValue after checking the driver's custom types. Am I misreading or misunderstanding something? Thanks.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Oct 17, 2016

I think they were meant for drivers to use for validation. Or maybe it was an internal detail and we didn't have internal packages at the time. I forget.

You can update the docs on it.

@kardianos
Copy link
Contributor Author

@kardianos kardianos commented Oct 17, 2016

@bradfitz I would actually prefer to remove the sql/driver uses of it, and request drivers use it to validate their own types. At least, I think that would be one of the required types to support custom database types. I just don't see the value of this call. If the driver doesn't support the parameter input type, let the driver throw the error.

The second option would be to add a new interface for the driver.Stmt, interface would be in charge of calling IsValid after checking custom driver types. So we could retain the current behavior until the driver adds the checks themselves.

No POC yet. Thoughts?

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Oct 17, 2016

Well, the idea was we wanted to make it easier to write drivers and not require drivers to deal with every one of Go's integer type sizes and signedness. That's why there's only int64 not uint16, int32, etc.

So we still want to convert the common things (like uint16 to int64) for drivers. But if a driver wants to opt-in to other types (likeuint64` or slices of things or arbitrary things like GPS coordinates), that's fine.

@kardianos
Copy link
Contributor Author

@kardianos kardianos commented Nov 28, 2016

Getting column types has been submitted to master.
I'm closing this issue as the remaining points are contained in #13567 and #16235 .

@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
6 participants
You can’t perform that action at this time.