persistent-sqlite should depend on direct-sqlite

[joeyadams]

The persistent-sqlite package currently contains

A port of the direct-sqlite package for dealing directly with PersistValues.

But copied code leads to copied issues, such as:

• The bundled sqlite3.c falling out of date
• A mixup in the Error enumeration

Can we make persistent-sqlite import its low-level functionality from direct-sqlite, rather than copying it? I'd be happy to do the integration work.

[IreneKnapp]

For whatever it's worth, as the author of direct-sqlite I consider it to be still maintained and would love to hear what you feel needs changing in it.

[snoyberg]

It's been a while, so I don't remember my reasons at the time, but I think it was because I wanted to allow embedding of the sqlite3.c file instead of depending on the system-installed one, as that simplifies Windows installation. Another major change is that persistent-sqlite uses the PersistValue datatype directly instead of having to perform an extra conversion step. I see that in more recent versions the source file was added, so that's at least one issue ticked off the list.

So @joeyadams: If you want to take a stab at making the move, I give no objections. One thing to point out in advance is that I had to implement a hack in the amalgamation itself to get GHCi to work on Linux, see d7daf0b

[IreneKnapp]

I agree that the intermediate value is a problem and have been thinking about what to do about it. Anyway, cool, glad to hear this can move ahead. :)

[joeyadams]

What if we create an even lower-level binding (we could call it "direct-sqlite-bindings") ? One that does not perform datum conversion or throw exceptions, but does translate constants like error codes and column types. This way, conversion and error handling strategy can be decided at a higher level.

By the way, I spotted this in persistent-sqlite:

reset :: Statement -> IO ()
reset statement = do
  error <- resetError statement
  case error of
    ErrorOK -> return ()
    _ -> return () -- FIXME confirm this is correct sqlError Nothing "reset" error
...
finalize :: Statement -> IO ()
finalize statement = do
  error <- finalizeError statement
  case error of
    ErrorOK -> return ()
    _ -> return () -- sqlError Nothing "finalize" error

Why is the error handling commented out here?

I have a guess. If the most recent call to sqlite3_step returns an error code, sqlite3_reset and sqlite3_finalize returns it as well. This isn't necessarily "wrong". My guess is that it's to guard against programmers forgetting to check and handle sqlite3_step. However, it might not do what we want if we're using exceptions for error handling. Consider the following scenario:

do stmt <- prepare db query
   bind stmt vals
   step stmt
 `catch` \(_ :: SomeException) -> do
   finalize stmt
   putStrLn "Rats, didn't work."

Here, finalize will throw the exception again, rather than silently free the statement.

[snoyberg]

I think the reason I implemented that was that calling reset or finalize changes the error message we end up getting, giving us a cryptic constraint message or some such instead of the actual error caused by the step.

[joeyadams]

direct-sqlite has been updated, but I'm still waiting for @IreneKnapp to decide if we're ready to release it yet.

I noticed that, when converting to Text, persistent-sqlite uses decodeUtf8With lenientDecode instead of just decodeUtf8. This means that if invalid UTF-8 is detected, it will silently clobber the data with Unicode replacement characters (U+FFFD) instead of throwing an exception. What's the rationale for this? Should direct-sqlite follow suit?

[meteficha]

decodeUtf8 is as evil as head: their exceptions are very unhelpful. So, either decodeUtf8With lenientDecode or decodeUtf8' with explicit error handling.

PS: persistent-mysql had very difficult to find bug where sometimes a decodeUtf8 error showed up. It took me a long time just to be sure that the bug was in persistent-mysql in the first place!

[joeyadams]

We could write our own variant of strictDecode that throws a UnicodeException with a more precise description:

strictDecode' _desc c = throw (DecodeError "Database.SQLite3.columnText: Invalid UTF-8" c)

This way, users can catch UnicodeException to handle invalid UTF-8 in the database.

We'll probably want to use lenientDecode for error messages, though.

[meteficha]

Yup, that'd be fine. It would give the same result as using decodeUtf8', right?

[joeyadams]

Yup, that'd be fine. It would give the same result as using decodeUtf8', right?

Here's how decodeUtf8' is implemented:

decodeUtf8' :: ByteString -> Either UnicodeException Text
decodeUtf8' = unsafePerformIO . try . evaluate . decodeUtf8With strictDecode

What I'm proposing is that, instead of catching a UnicodeException, modifying it to have a better description, and throwing a new UnicodeException, we just use a description containing "Database.SQLite3" in it to begin with.

So I guess it would be the same result as using decodeUtf8', but safer and more efficient.

[meteficha]

Eeks, I've never looked at decodeUtf8''s implementation before! That's nice to know.

[gregwebs]

@joeyadams are you still interested in moving this to use direct-sqlite ?

[snoyberg]

Closing out old issues, please reopen if still relevant