Explore relational database state storage#1465
Explore relational database state storage#1465bcardiff wants to merge 34 commits intohaskell:masterfrom
Conversation
seed the database
Depends on TemplateHaskell
Not really needed since they run in a different directory
So there is no need to override the convention
To have a more controlled migration
To use UserId directly we need to change it to a non machine-dependent size for Beam. MemSize and Pretty are not defined on Int32. But other places start to complain after that
Database.SQLite.Simple.execute_ is not able to run multiple statements at once
Enums are saved using Show and pared with Read They currently abort the request if the data is malformed
danbornside
left a comment
There was a problem hiding this comment.
i'm not any more familiar with hackage-server codebase than brian, only that we paired on this a bit and i'm more familiar with beam. Even with requested changes i'm not confident enough to approve as-is without some input from mantainers
| } madetails | ||
| let cdetails = change adetails | ||
|
|
||
| accountDetailsUpsert AccountDetailsRow { |
There was a problem hiding this comment.
this is a bit ormic. this should really use the fine grained syntax to affect only the relevant fields without needing to read it out of the db first.
| deriving (Eq, Ord, Read, Show, MemSize) | ||
|
|
||
| instance FromBackendRow Sqlite AuthToken where | ||
| fromBackendRow = AuthToken . BSS.toShort <$> fromBackendRow |
There was a problem hiding this comment.
does this need to be encoded? the corresponding column in the schema is TEXT; and i rather think that's not a binary-safe column type. sqlite supports BLOB and postgress has BINARY, which probably could work, although for a small value like this, they're pretty inconvenient compared to encoding to hex or base64 or something
There was a problem hiding this comment.
As mentioned elsewhere, you probably want to implement FromField rather than FromBackendRow instead
| @@ -0,0 +1,47 @@ | |||
| {-# LANGUAGE DeriveAnyClass #-} | |||
|
|
||
| type AccountDetailsRow = AccountDetailsT Identity | ||
|
|
||
| deriving instance Show AccountDetailsRow |
There was a problem hiding this comment.
instances for type aliases are a bit dodgy
There was a problem hiding this comment.
This is actually the idiomatic way to provide instances for Beam table types, for what it's worth
| sqlValueSyntax = autoSqlValueSyntax | ||
|
|
||
| instance FromBackendRow Sqlite AccountDetailsKind where | ||
| fromBackendRow = read . unpack <$> fromBackendRow |
There was a problem hiding this comment.
read will throw an exception if there's a parsing problem. either fail pure . Text.Read.readEither would be better.
supplying a meaningfull error message instead of Prelude.read no parse would be better still
There was a problem hiding this comment.
You should not have to define explicit instances for FromBackendRow. The class BeamBackend ultimately states that reading columns from Sqlite is done via Database.Sqlite.Simple.FromField.FromField.
Therefore, you should define an instance of Database.Sqlite.Simple.FromField.FromField AccountDetailsKind instead
LaurentRDC
left a comment
There was a problem hiding this comment.
I help maintain Beam. I provided some comments on idiomatic Beam usage. Feel free to summon me in this PR and others if this helps
| sqlValueSyntax = autoSqlValueSyntax | ||
|
|
||
| instance FromBackendRow Sqlite AccountDetailsKind where | ||
| fromBackendRow = read . unpack <$> fromBackendRow |
There was a problem hiding this comment.
You should not have to define explicit instances for FromBackendRow. The class BeamBackend ultimately states that reading columns from Sqlite is done via Database.Sqlite.Simple.FromField.FromField.
Therefore, you should define an instance of Database.Sqlite.Simple.FromField.FromField AccountDetailsKind instead
| runSelectReturningOne :: forall a. (FromBackendRow Sqlite a) => SqlSelect Sqlite a -> Transaction (Maybe a) | ||
| runSelectReturningOne q = | ||
| Transaction $ ReaderT $ \(SqlLiteConnection conn) -> runBeamSqlite conn $ Database.Beam.runSelectReturningOne q | ||
|
|
||
| runInsert :: forall (table :: (Type -> Type) -> Type). SqlInsert Sqlite table -> Transaction () | ||
| runInsert q = | ||
| Transaction $ ReaderT $ \(SqlLiteConnection conn) -> runBeamSqlite conn $ Database.Beam.runInsert q | ||
|
|
||
| newtype Transaction a = Transaction {unTransaction :: ReaderT Connection IO a} | ||
| deriving (Functor, Applicative, Monad) | ||
|
|
||
| runTransaction :: Transaction a -> Connection -> IO a | ||
| runTransaction t = runReaderT (unTransaction t) |
There was a problem hiding this comment.
I understand that the plan is to support both Sqlite and Postgres. I think this can be done by abstracting over the backend instead of using Sqlite specifically here. Then, either via ExistentialQuantification or some typeclass, the only database-specific bit is the runBeamPostgres vs runBeamSqlite functions
| forM_ (Users.enumerateAllUsers users) $ \(uid, uinfo) -> do | ||
| let (status, authInfo) = | ||
| case userStatus uinfo of | ||
| AccountEnabled a -> (Enabled, Just a) | ||
| AccountDisabled ma -> (Disabled, ma) | ||
| AccountDeleted -> (Deleted, Nothing) | ||
|
|
||
| Database.runInsert $ | ||
| insert | ||
| (_tblUsers Database.hackageDb) | ||
| (insertValues [UsersRow { | ||
| _uId = uid, | ||
| _uUsername = userName uinfo, | ||
| _uStatus = status, | ||
| _uAuthInfo = authInfo, | ||
| _uAdmin = Group.member uid admins | ||
| }]) | ||
|
|
||
| forM_ (Map.toList (userTokens uinfo)) $ \(token, desc) -> do | ||
| Database.runInsert $ | ||
| insert | ||
| (_tblUserTokens Database.hackageDb) | ||
| (insertExpressions [UserTokensRow { | ||
| _utId = default_, | ||
| _utUserId = val_ uid, | ||
| _utToken = val_ token, | ||
| _utDescription = val_ desc | ||
| }]) |
There was a problem hiding this comment.
It might be more performant to execute runInsert fewer times, but each time with more than a single value.
| deriving (Eq, Ord, Read, Show, MemSize) | ||
|
|
||
| instance FromBackendRow Sqlite AuthToken where | ||
| fromBackendRow = AuthToken . BSS.toShort <$> fromBackendRow |
There was a problem hiding this comment.
As mentioned elsewhere, you probably want to implement FromField rather than FromBackendRow instead
| instance FromBackendRow Sqlite UsersStatus where | ||
| fromBackendRow = read . unpack <$> fromBackendRow |
There was a problem hiding this comment.
You should define an instance of FromField rather than FromBackendRow
| instance FromBackendRow Sqlite UserId where | ||
| fromBackendRow = UserId . fromIntegral @Int32 <$> fromBackendRow |
There was a problem hiding this comment.
You should derive an instance of FromField rather than FromBackendRow
| instance FromBackendRow Sqlite UserName where | ||
| fromBackendRow = UserName . unpack <$> fromBackendRow |
There was a problem hiding this comment.
Same here, FromField instead of FromBackendRow
| instance FromBackendRow Sqlite UserAuth where | ||
| fromBackendRow = UserAuth . PasswdHash . unpack <$> fromBackendRow |
There was a problem hiding this comment.
FromField instead of FromBackendRow
|
Thanks for the feedback @LaurentRDC . I wasn't aware of FromField. The tutorial 3 and other examples I found online always used Before jumping into Maybe this is not the right place to have this conversation since it's a "how to use beam" kind of question. If the answer is not trivial we can move to haskell forum or somewhere else this bit. |
|
You're right that the Beam tutorial shows an explicit implementation of
If you can express this as a newtype, then this is where things are most ergonomic. For example, for a newtype UserId = MkUserId Int32
deriving stock (Show)
deriving newtype (Postgres.FromField, Sqlite.FromField)
instance FromBackendRow Sqlite UserId
instance FromBackendRow Postgres UserIdI think this is the simplest way to have the representation of |
This is the exploration done during AmeriHac to migrate the acid store to a relational database. I will leave it as draft since it's an exploration probably that will trigger discussions rather than a patch. I also hope to continue soon to keep migrating more state to the database to keep validating the approach.
The goals were
Things not covered
Other general doubts
To keep the exploration shorter we assumed that db backups would probably be done using the usual db backup tools and not via CSV files. It would be simpler to aim for full code migration and no choice but to migrate to the database. If these assumptions are not right then we will need some abstractions to keep reading and writing to the store (acid or db) which increases the effort.
This is the first time using Beam for me, so there might be better ways of doing things.
Thanks to @danbornside for all the pairing on this