lib / pq

Pure Go Postgres driver for database/sql
https://pkg.go.dev/github.com/lib/pq
MIT License
8.86k stars 908 forks source link

inet, cidr, macaddr support #121

Open georgyo opened 10 years ago

georgyo commented 10 years ago

Just wondering if other postgres types are planned to be supported? Specifically inet, cidr and macaddr.

Similar to time.Time, if stored in the database that way, its also most likely better to have a better in code representation of it.

Something along the lines of: https://gist.github.com/georgyo/6143350

The biggest problem I see is that these types currently get caught by the return s statement, meaning that this has the possibility to break existing code... Still curious because of time.Time.

gucki commented 8 years ago

:+1:

petergoldstein commented 8 years ago

@johto Is this an issue that is of interest to the library maintainers? I'm asking because it's clearly a very old issue (2+ years), and doesn't seem to have received any attention during that time.

I have a need for this functionality, and would be open to scheduling some time to contribute to the project. But before I devote the time, I'd like to know if there's a realistic chance to get it merged in.

On a related note, is the backwards compatibility concern raised by @georgyo in his initial comment a real concern for you? As is it seems like the pq library just doesn't handle queries against those columns properly at all, so any code that relied on existing behavior would be problematic at best.

@johto If you indicate support for a possible PR (subject to review of course) I'm happy to contribute one with necessary tests, etc.

johto commented 8 years ago

@johto Is this an issue that is of interest to the library maintainers? I'm asking because it's clearly a very old issue (2+ years), and doesn't seem to have received any attention during that time.

I've not needed to interpret inet or cidr types from any Go application, so I've not spent any time on this myself. But the fact that it hasn't received any attention from me doesn't really matter; I've spent a lot of time merging in features I've never used and probably never will. The fact that this issue has been open for two years suggests that most people don't care about this feature, or at least not enough to contribute it.

On a related note, is the backwards compatibility concern raised by @georgyo in his initial comment a real concern for you? As is it seems like the pq library just doesn't handle queries against those columns properly at all, so any code that relied on existing behavior would be problematic at best.

There's nothing problematic with the current behavior. You can scan any "custom" type into a string and then post-process to do whatever you need with it. Breaking that is not only out of the question, but in violation of the database/sql/driver interface:

// Next is called to populate the next row of data into
// the provided slice. The provided slice will be the same
// size as the Columns() are wide.
//
// The dest slice may be populated only with
// a driver Value type, but excluding string.
// All string values must be converted to []byte.

@johto If you indicate support for a possible PR (subject to review of course) I'm happy to contribute one with necessary tests, etc.

This should really be implemented using types which implement Scanners and Valuers if the code wants to be in pq. Similarly to how e.g. hstore support is implemented currently.

petergoldstein commented 8 years ago

@johto Ok, I think I understand the appropriate approach that's consistent with the database/sql/driver interface. Hstore seems fairly straightforward, and I think I can simply mimic the approach taken for NullBool, NullFloat64, etc. to handle nulls for the network address types. Let me take a pass at this and submit a PR.