Skip to content

Conversation

BardurArantsson
Copy link
Contributor

@BardurArantsson BardurArantsson commented Apr 20, 2017

Hi,

So this is the gist of what I want to do. It doesn't actually compile right now due to circular dependencies. (My word, how did this code get so tangled up in circular dependencies?)

Obviously it's missing documentation and I need to actually get it compiling + prettify it a little bit. Wrt. getting it compiling... is it OK if I do a little moving around of code in commits leading up to this? (If you want I can try to be extra careful to preserve the interface and just move a few implementations around and re-export to preserve the old interface. Or we could just bump versions according to PVP.)

What do you think?

EDIT: Obviously the only thing that makes sense is to view the full diff. Individual commits don't make any sense ATM.

-- FIXME: ...withParser + create "plain" one with FromRow r instead
fetchForward :: RowParser r -> Cursor -> Int -> (a -> r -> IO a) -> a -> IO a
fetchForward parser (Cursor name conn) chunkSize f a0 = do -- FIXME: escape name!
let q = toByteString (byteString "FETCH FORWARD "
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a very minor difference here wrt. the original implementation in foldWithParserBlahblah: We construct this string every time around the loop, but seeing as we're fetching from a database (thus already incurring the cost of I/O), I don't think it's a big other. Alternatively one could specify the chunkSize up front when creating the cursor, but then one loses the possibility of using an adaptive algorithm.

@BardurArantsson
Copy link
Contributor Author

BardurArantsson commented Apr 20, 2017

EDIT: Scratch that.

~~Ok, so I've cleaned most of it up. All that's missing is fixing an infinite loop in doFold. The problem is pretty obvious: With the given API, there's no way of signaling a "done" condition from "fetchForward*", so I'll add that. (It may be a day or two before I get back to this. Any comments would be appreciated.)

Btw, I think everything up to the ff43f70 commit should be good to go, so feel free to merge that. I'd basically consider 8724be7 it a clean up that's worth it regardless of whatever else happens to this PR.~~

@BardurArantsson
Copy link
Contributor Author

Ok, should be good to go, modulo comments... though the PR looks kind of messed up right now. Not sure what happened. I'll open a new one since you haven't commented on anything yet.

@lpsmith
Copy link
Owner

lpsmith commented Apr 24, 2017

(My word, how did this code get so tangled up in circular dependencies?)

LOL! Honestly I'm not entirely sure. It just sorta, happened. I think the current implementation of the TypeInfo system is one of the bigger culprits, because parsing the results of query can result in further queries to the pg_type metatable, and I didn't want to have to implement these queries in terms of libpq. (Perhaps I should, just to get rid of some of the circularity)

Also, there's a less obvious circular dependency in the TypeInfo.Static module, which is generated using postgresql-simple, and is necessary for postgresql-simple to function. I think I originally created the predecessor module with a script that used Chris Done's pgsql-simple, which required almost no effort to port over to postgresql-simple. But it would be a bad idea to dynamically generate TypeInfo.Static as part of a regular build process, because well then you have some nasty circular dependencies to work with, and you need access to a running instance of postgres.

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