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: decode time.Time into *time.Time #22300

Closed
achille-roussel opened this issue Oct 17, 2017 · 10 comments
Closed

database/sql: decode time.Time into *time.Time #22300

achille-roussel opened this issue Oct 17, 2017 · 10 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@achille-roussel
Copy link
Contributor

achille-roussel commented Oct 17, 2017

What version of Go are you using (go version)?

go version go1.9.1 darwin/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/achilleroussel/dev"
GORACE=""
GOROOT="/usr/local/go"
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/0p/tfjq063j4hdcjnm0_9qb4bww0000gn/T/go-build931864653=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"

What did you do?

Converting from a time.Time value returned by the SQL driver into a time.Time value uses a code path based on reflection, where it could be a simple assignment. Is there any reason why the database/sql package would not want to directly assime the time values?
(none that I can think of right now).

What did you expect to see?

I believe we could add this as an optimization to decode time values into time values (which is a very common case):

case *time.Time:
    if d == nil {
        return errNilPtr
    }
    *d = s
    return nil

What did you see instead?

https://github.com/golang/go/blob/master/src/database/sql/convert.go#L271-L288

	case time.Time:
		switch d := dest.(type) {
		case *string:
			*d = s.Format(time.RFC3339Nano)
			return nil
		case *[]byte:
			if d == nil {
				return errNilPtr
			}
			*d = []byte(s.Format(time.RFC3339Nano))
			return nil
		case *RawBytes:
			if d == nil {
				return errNilPtr
			}
			*d = s.AppendFormat((*d)[:0], time.RFC3339Nano)
			return nil
		}

(notice there are only cases to decode time values into string or byte slices)

@davecheney
Copy link
Contributor

It's unusual to use values of type *time.Time. What is your use case?

@achille-roussel
Copy link
Contributor Author

What's unusual about it? This seems like the most type-safe approach to loading time values from a SQL database.

My use case is this:

  • I have a MySQL table with datetime columns
  • I select rows from that table with QueryContext + Scan, passing address of time variables to load values from the datetime columns.
  • Those columns are creation/update times of the objects represented by each rows, this also seems to me like a very common schema found in databases.

Decoding into time.Time is currently supported by the database/sql package as it goes through the reflection path where the source and destination types are tested with AssignableTo https://github.com/golang/go/blob/master/src/database/sql/convert.go#L365

It would be way more efficient to just have a special case for *time.Time destinations.

The reason I'm raising this issue is because I have a production service which has a significant amount of memory allocated in convertAssign:

screen shot 2017-10-16 at 6 00 03 pm

@achille-roussel
Copy link
Contributor Author

achille-roussel commented Oct 17, 2017

Simplified code example of what I meant:

rows, err := db.QueryContext(ctx, "select ..., created_at, updated_at from ...") 
if err != nil {
    return err
}

var ...
var createdAt time.Time
var updatedAt time.Time

for rows.Next() {
    err := rows.Scan(..., &createdAt, &updated) // here are the *time.Time
    ...
}

@davecheney
Copy link
Contributor

davecheney commented Oct 17, 2017 via email

@achille-roussel
Copy link
Contributor Author

Indeed, my point isn't that it doesn't work, it's that the database/sql package implements it very inefficiently, and it seems like programs have no alternative to work around the limitation here.

Cycling back, my original question was: is there a reason that the current implementation didn't special case the assignment from time.Time to time.Time values? I can't think of any but maybe I'm missing some context here.

If there are no reasons I'll be happy to contribute a patch.

@ianlancetaylor
Copy link
Contributor

CC @kardianos

@ianlancetaylor ianlancetaylor added this to the Go1.10 milestone Oct 17, 2017
@ianlancetaylor ianlancetaylor added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 17, 2017
@ianlancetaylor
Copy link
Contributor

Seems reasonable to me, given that we already have special handling of time.Time.

@kardianos
Copy link
Contributor

SGTM. I'll send a CL soon.

@kardianos kardianos self-assigned this Oct 17, 2017
@achille-roussel
Copy link
Contributor Author

Thanks for the quick responses you all!

@gopherbot
Copy link

Change https://golang.org/cl/73232 mentions this issue: database/sql: scan into *time.Time without reflection

@golang golang locked and limited conversation to collaborators Oct 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

5 participants