Support Postgresql array types

[truongsinh]

#49

This PR has support for:

• scanning text[], varchar[], int_[], float_[]
• valuing text[], varchar[], int_[], float_[] in statement.Exec/Query/QueryRow only

Known limitation: slice type cannot be converted if used directly by db.Exec/Query/QueryRow

[johto]

This PR has support for:

I think the first thing here would be to split this into two separate patches, unless there's a really good reason not to. These two seem to be entirely separate functionality, and conflating them makes for a tough review process.

valuing text[], varchar[], int_[], float_[] in statement.Exec/Query/QueryRow only

I'm guessing this is because of the comment I made in #49 in February? Prepared statements are quite evil so I'd like to avoid this limitation if possible; or at least provide a workaround, if nothing else. It's not clear how my suggestion in #49 would work if we wanted to send arrays over in binary format should #209 ever happen, though.

[truongsinh]

I think the first thing here would be to split this into two separate patches, unless there's a really good reason not to. These two seem to be entirely separate functionality, and conflating them makes for a tough review process.

My reason is that this PR supports array types. And from my logic, Scanner and Valuer interface always go hand-in-hand and have 1-to-1 map. In other word, if I can bind []string, I expect I can scan []string too (it was not the case when I reused another PR while doing this PR). Of course I am happy to hear your opinion of separating Scanner and Valuer interface.

Prepared statements are quite evil

Did not know that!? Is is because of performance - 2 roundtrips (which is from your comment in #49). It is not a problem in all of my projects so far. The way we have been doing it, is to prepare all statements at init time, and only call *stmt.Exec/Query/QueryRow at runtime. I think this approach is recommended for security (no/less injection), integrity (panic at init time if there are typos or entity not exists) and performance (no parsing require at runtime).

[truongsinh]

Talking of workaround of db.Exec/Query/QueryRow, I think https://github.com/jmoiron/sqlx or some higher wrapper of Go's sql is the only way, because of this line http://golang.org/src/pkg/database/sql/sql.go?s=23949:23985#L884

[johto]

Talking of workaround of db.Exec/Query/QueryRow, I think https://github.com/jmoiron/sqlx or some higher wrapper of Go's sql is the only way, because of this line http://golang.org/src/pkg/database/sql/sql.go?s=23949:23985#L884

https://gist.github.com/johto/2914cd7309dedc5fb0d8 seems to be working fine; am I missing something?

[truongsinh]

I think you mean func (a IntArray) Value() should do some string join and concatenation, instead of hard-coding '{1,2,3}', and IntArray should be []interface for generic use? Anyway, i think with this approach, we are moving the logic of converting from driver to user; and still lack of Scanner.

[johto]

I think you mean func (a IntArray) Value() should do some string join and concatenation, instead of hard-coding '{1,2,3}'

Yes; it's a proof-of-concept.

, and IntArray should be []interface for generic use?

[]interface{} is horrible for "generic use". interface{} might work; I'd have to see how bad of an effect all of the reflection magic would have on performance. Or there could be specific types for the common types (Int, Time, etc.) and one reflection-based "generic" array.

Anyway, i think with this approach, we are moving the logic of converting from driver to user;

Maybe. But it works with Query(), which is the point I'm trying to make. If we can make passing arrays directly work for prepared statements, fine. But I have no personal interest in that kind of functionality, and I'm sure I'm not the only one.

and still lack of Scanner.

Scanning is still a separate feature in my mind. We need to keep the discussions separate, and find the best solutions to both problems.

[johto]

My reason is that this PR supports array types. And from my logic, Scanner and Valuer interface always go hand-in-hand and have 1-to-1 map. In other word, if I can bind []string, I expect I can scan []string too (it was not the case when I reused another PR while doing this PR). Of course I am happy to hear your opinion of separating Scanner and Valuer interface.

Perhaps it's not necessary for you to go and rip this patch into two pieces right now, but both interfaces need to be reviewed separately.

Prepared statements are quite evil

Did not know that!? Is is because of performance - 2 roundtrips (which is from your comment in #49). It is not a problem in all of my projects so far.

No, prepared statements are actually better in that area in pq, since they only need to do a single roundtrip for execution. But they're a nightmare to deal with when you have live schema changes going on under the app since the server can't invalidate some caches automatically in a safe manner, so you need to do it yourself. I doubt any Go apps actually do this, so they'll just break until they're restarted.

The way we have been doing it, is to prepare all statements at init time, and only call *stmt.Exec/Query/QueryRow at runtime. I think this approach is recommended for security (no/less injection)

You don't need prepared statements for that, just parameterization. At least in pq, db.Query() is just as safe against injection as stmt.Query().

, integrity (panic at init time if there are typos or entity not exists)

Not necessarily a feature for all apps..

and performance (no parsing require at runtime).

This can often be true, though.

[johto]

Anyway, i think with this approach, we are moving the logic of converting from driver to user;

Maybe. But it works with Query(), which is the point I'm trying to make.

Sorry I was a bit unclear on this; I mean it works with db.Query(), i.e. without prepared statements. Note that I'm not suggesting this as the primary interface right now, but as a workaround for the lack of direct value-passing support for anything other than prepared statements.

But that might change once I dive deeper into this patch (which I really have not done at all).

[truongsinh]

Thanks for the insight, then I think from this I will make a separate PR, scanning (as we are still looking for good-enough workaround for valuing)

[bgentry]

Quick question... Is this desired? And does it work as expected if you use []sql.NullString instead of []string? Maybe add a test using the sql.NullString type?

[johto]

Not being able to distinguish between an empty string and a NULL in an array element is not going to fly. Trying to scan an array with NULL elements into a []string ought to return an error.