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

Added generic ToField and FromField classes #196

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

zohl
Copy link

@zohl zohl commented Oct 25, 2016

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:

  • typename equals to real type name

    We are still able to retrieve metadata of the type, but this will be
    a performance hit as we will have extra query for each execution of
    the parser.

  • typename equals to "record"

    We don't have any data, so there is nothing to do! Even worse, we
    can gen "record" instead of our custom type any time when we
    forget to cast results to it.

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
Copy link
Owner

lpsmith commented Oct 26, 2016

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
Copy link
Author

zohl commented Oct 26, 2016

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
Copy link
Owner

lpsmith commented Oct 26, 2016

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
Copy link
Author

zohl commented Oct 26, 2016

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants