Extending SqlError type with additional fields

[sopvop]

Though, it will break api and maybe some library clients, but I think it can be valuable addition. Maybe this can be done in next major version of postgresql-simple.

libgsql docs show what you can actually get different parts of error message from postgres, using PQresultErrorField function from libpq, instead of concatenated errors from connection object.
So, if SqlError gets additional fields like sqlErrorDetail, sqlErrorHint and then

sqlErrorMsg <- PG_DIAG_MESSAGE_PRIMARY
sqlErrorDetal <- PG_DIAG_MESSAGE_DETAIL
sqlErrorHint <- PG_DIAG_MESSAGE_HINT

For example HINT is often used to communicate error detail from database stored functions to client in some kind of parseable format, like with foreign key violation the default trigger sets hint to "user_id=(1337)". AFAIK it is not subject to localization.
Also user written functions may print something relevant there.

I'm willing to implement this :)

Edit: libpq bindings used by posgresql-simple proved all required functionality.
Edit2: And think you know that :)

[lpsmith]

Actually I didn't know that, but that is what I expected. As far as I am currently aware, postgresql-libpq isn't missing anything other than a few obscure and rarely-used functions such as PQprint and PQtrace.

I'm not really opposed to simply changing SqlError, though I suppose keeping the existing fields would mean that some things wouldn't break. Though I suspect a lot of code wouldn't break even if we completely change it, and I suspect the class of code currently in existence that would break if we throw out the existing fields but not break if we didn't is probably pretty small.

If this is what you think needs to be done, then I'm down for it.

You might go through and search for FIXME in the source; a lot of those are where where error messages may need improvement. This isn't an issue I've investigated throughly, so I appreciate you looking into it.

[joeyadams]

@lpsmith: Regarding compatibility with pull request #34, as long as we keep sqlState (or at least some way to test if serialization_failure occurred), I'm a happy camper.

Also, maybe we should replace sqlNativeError with:

sqlExecStatus :: Maybe PQ.ExecStatus

Currently, ExecStatus is the only thing sqlNativeError is used for:

throwResultError :: ByteString -> PQ.Result -> PQ.ExecStatus -> IO a
throwResultError context result status = do
    errormsg  <- maybe "" id <$> PQ.resultErrorMessage result
    statusmsg <- PQ.resStatus status
    state     <- maybe "" id <$> PQ.resultErrorField result PQ.DiagSqlstate
    throwIO $ SqlError { sqlState = state
                       , sqlNativeError = fromEnum status
                       , sqlErrorMsg = B.concat [ context, ": ", statusmsg
                                                , ": ", errormsg ]}

[lpsmith]

Right, my understanding is that sopvop intended on keeping sqlState as-is. I was mostly curious if you were dependent on anything else in code that isn't public.

The current SqlError was basically adopted from HDBC, by the way. I really am not opposed to overhauling it. It would probably be a good idea to keep sqlState as is, but changing sqlNativeError and sqlErrorMsg is fine by me.

[sopvop]

moving to #35