lpsmith / postgresql-simple

Mid-level client library for accessing PostgreSQL from Haskell
Other
206 stars 71 forks source link

Added generic ToField and FromField classes #196

Open zohl opened 7 years ago

zohl commented 7 years ago

This patch adds default generic-based implementations for ToField and FromField classes. These are useful when you need to retrieve results from sql-functions returning setof some_type or from sql-queries returning columns with records.

I should mention one major change in a core module as I'm not sure whether it's acceptable for you (and the community) or not.

The short story: in order to compose existing parsers into a record parser we need to bypass typecheck of a Field variable. Therefore typeOid in a Field record was changed from PQ.Oid to Maybe PQ.Oid.

The long story: when we parse a record value we have no information about types of it's fields. More precisely there are two cases:

Hence to parse a record value we have to rely on a corresponding haskell type. We already have a lot of good parsers for base types and it would be unforgivable not to employ them :) But there is a problem: all of them are checking Field variable for oid (or typename), which in our case is far from required value. So (from my point of view) it's naturally to make typeOid field optional.

Aside of type unsafety there might be another pitfalls like degraded performance or excessive memory consumption, but I'm neither expert in haskell nor familiar with internals of postgresql-simple to say for sure they aren't the cases here.

All tests were passed against PostgreSQL v.9.5.1.

lpsmith commented 7 years ago

This looks interesting, but for starters, why are you making a (Maybe Oid) everywhere?

And I hate to say it, but we probably want to use a sentinel Oid for Nothing instead of creating a heap-allocated object for each Oid. I think postgresql has a sensible Oid that we can use as a sentinel.

zohl commented 7 years ago

Once I changed type of typeOid I had to change every parser to bypass typecheck on Nothing and some functions needed to be adjusted to return Maybe PQ.Oid instead of pure value. I cannot say I was happy with it, a sentinel value could be better approach. I'll research on postgresql oids for a proper value a little bit later.

Also I see travis is complaining (mostly because I forgot to remove my db credentials from the connection string), so there is still room for improvements.

lpsmith commented 7 years ago

That didn't exactly answer my question, why is it even necessary (in your mind) to move to Maybe Oid, (regardless of whether it's represented with a Maybe or a sentinel value)?

If it's in order to support the record type, I think the best thing to do at the moment, unfortunately, is to not support the record type for the time being. Of course, there should be no problems supporting composite types with the current state of postgresql-simple, it's just the less-typed record type that's problematic.

unknown may be the appropriate sentinel value to use in place of Nothing, in which case one possible way to support the record type might be to allow the unknown Oid in most of the FromField conversions. But honestly, in the longer run, I really do need to seperate the typechecking logic from the parsing logic, (and I want to hoist typechecking to once per Result instead of once per row) which would allow for not typechecking things when we don't have type information, without mucking up the type checking logic all over the place.

zohl commented 7 years ago

Sorry for being unclear. The only reason for that change was bypassing typename/typeoid checks in parsers. And changing the type was the shortest way to achieve it. Thanks to your explanation, now I understand that this wasn't the right decision at all.

And I have to say nothing but that you are completely right, for my implementation looked like rather a hacky solution. If we were able to turn off typechecks without involving core structures, it would be more convenient way to deal with parsers.

unknown seems like just what we need, according to this[1] discussion:

the type initially imputed to unadorned string literals

I've switched implementation to using this typeoid, but adjusted only few parsers in order to pass the tests. Still looks hacky, though this can be the most optimal way at the moment.

[1] https://www.postgresql.org/message-id/183.1302200970%40sss.pgh.pa.us