#389 jsonb #390

willbasky · 2019-08-29T14:51:43Z

Resolve #389

This change is

neongreen · 2019-08-31T15:02:02Z

back/benchmarks/Main.hs

@@ -0,0 +1,49 @@
+-- | Module contains all stuff to migrate from AcidState to Postgres.


Wrong description?

neongreen · 2019-08-31T15:02:55Z

back/benchmarks/Main.hs

+        "Database"
+        [ bench "select" $ nfIO $
+            runTransactionExceptT conn Read $ selectCategory "category1111"
+        , bench "updete" $ nfIO $


Suggested change

, bench "updete" $ nfIO $

, bench "update" $ nfIO $

neongreen · 2019-08-31T15:03:48Z

back/benchmarks/Main.hs

+      bgroup
+        "Database"
+        [ bench "select" $ nfIO $
+            runTransactionExceptT conn Read $ selectCategory "category1111"


Please let's not have such names in code, they look sloppy. "category" or "uid1" would be better.

length "category1111" == 12 as Uid needed.

neongreen · 2019-08-31T15:04:15Z

back/benchmarks/Main.hs

+
+main :: IO ()
+main = do
+    conn <- connect


We have two-space indentation.

neongreen · 2019-08-31T15:06:01Z

back/benchmarks/Main.hs

+            runTransactionExceptT conn Write $ updateCategory "category1111" update
+        ]
+    update :: Category -> Category
+    update = _categoryTitle .~ "title10"


After the first application of update the title will be "title10", so update will be equivalent to id and updateCategory will not do any writes. You should do something that would change the category every time, e.g. _categoryTitle <>~ "+", and then re-run the benchmarks.

ok. then speed is

neongreen · 2019-08-31T15:30:49Z

back/src/Guide/Types/Core.hs

@@ -74,6 +76,20 @@ instance Aeson.ToJSON Trait where
  toJSON = Aeson.genericToJSON Aeson.defaultOptions {
    Aeson.fieldLabelModifier = over _head toLower . drop (T.length "trait") }

+instance Aeson.FromJSON Trait where


If this instance is not using generic, the ToJSON instance should also not use generic.

neongreen · 2019-08-31T15:31:35Z

back/src/Guide/Types/Core.hs

+instance Aeson.FromJSON Trait where
+  parseJSON = Aeson.withObject "Trait" $ \o -> do
+    traitUid     <- o Aeson..: "uid"
+    content      <- o Aeson..: "content"


Since MarkdownInline has a FromJSON instance, you can just write traitContent <- o Aeson..: "content".

neongreen · 2019-08-31T15:31:48Z

back/src/Guide/Types/Core.hs

+    traitContent <- toMarkdownInline <$> content Aeson..: "text"
+    pure Trait{..}
+
+instance ToPostgres Trait where


Is this instance used anywhere?

neongreen · 2019-08-31T15:32:34Z

back/src/Guide/Types/Core.hs

+    pure Trait{..}
+
+instance ToPostgres Trait where
+  toPostgres = toByteString . Aeson.encode >$< HE.jsonbBytes


Why are you using jsonbBytes instead of just jsonb? (Same for all other instances.)

neongreen · 2019-08-31T15:33:22Z

back/src/Guide/Types/Core.hs

+    itemCreated     <- o Aeson..: "created"
+    itemHackage     <- o Aeson..:? "hackage"
+    summary         <- o Aeson..: "summary"
+    itemSummary     <- toMarkdownBlock <$> summary Aeson..: "text"


Again, toMarkdownBlock is not needed because MarkdownBlock already has a FromJSON instance.

neongreen · 2019-09-02T17:30:01Z

back/package.yaml

+      - hasql-transaction
+
+    ghc-options:
+    - -O


I think this won't have any effect if the library is not compiled with -O, so you can just remove it and add a short instruction on how to run benchmarks properly to the README.

neongreen · 2019-09-02T17:30:17Z

back/src/Guide/Database/Import.hs

 import Guide.Config
 import Guide.Logger


-- | Load categories and deleted categories from acid state to postgres
+-- | Load categories and archives categories from acid state to postgres


Suggested change

-- | Load categories and archives categories from acid state to postgres

-- | Load categories and archived categories from acid state to postgres

Is this what you meant?

neongreen · 2019-09-02T17:30:57Z

back/src/Guide/Database/Import.hs

-      <- runTransactionExceptT conn Read getCategories
+    catPostgres <- runTransactionExceptT conn Read $
+      selectCategories (#archived False)
+    catarchivedPostgres <- runTransactionExceptT conn Read $


Suggested change

catarchivedPostgres <- runTransactionExceptT conn Read $

catArchivedPostgres <- runTransactionExceptT conn Read $

neongreen · 2019-09-02T17:31:05Z

back/src/Guide/Database/Import.hs

    let checkedCatDeleted =
-          sortOn categoryUid catDeletedPostgres ==
+          sortOn categoryUid catarchivedPostgres ==


Suggested change

sortOn categoryUid catarchivedPostgres ==

sortOn categoryUid catArchivedPostgres ==

neongreen · 2019-09-02T17:35:54Z

back/src/Guide/Database/Queries/Update.hs

+  -> "archived" :! Bool
+  -> ExceptT DatabaseError Transaction ()
+updateCategoryArchived catId (arg #archived -> archived) = do
+  isArchived <- isCategoryArchived catId


"old_archived" and "new_archived" would be more consistent.

neongreen · 2019-09-02T17:39:09Z

back/src/Guide/Markdown.hs

+instance Aeson.FromJSON MarkdownTree where
+  parseJSON = Aeson.withObject "MarkdownTree" $ \o -> do
+    txt <- o Aeson..: "text"
+    prefix <- o Aeson..:? "prefix" Aeson..!= T.empty


You can use "" instead of T.empty, I don't think there is a reason to not use overloaded strings here.

neongreen · 2019-09-02T17:48:03Z

back/src/Guide/Types/Core.hs

+-- | Unwarp result to category or fail.
+resultToEither :: Aeson.Result Category -> Category
+resultToEither (Aeson.Success category) = category
+resultToEither (Aeson.Error s) = error $ "fromJSON failed with error: " ++ s


No, error is verboten. Apparently we'll have to use jsonbBytes after all because it would let us give back the decoding error (as Left) without using error.

Comment legaxy code

willbasky requested a review from neongreen August 29, 2019 15:36

neongreen reviewed Aug 31, 2019

View reviewed changes

willbasky force-pushed the willbasky/jsonb branch from ee90723 to 4f0c0a9 Compare September 2, 2019 05:42

neongreen reviewed Sep 2, 2019

View reviewed changes

willbasky self-assigned this Sep 4, 2019

neongreen force-pushed the willbasky/jsonb branch from 761ee02 to 18cc4b9 Compare September 9, 2019 11:39

willbasky added 9 commits September 10, 2019 16:05

Change schema, implement functions

ba9eee8

Add benchmarks

77ffda6

Update import module.

b136aa4

Comment legaxy code

Fix markdown instances. Remove unused instances

c01f511

jsonbbytes to jsonb

397a2b9

Add comment

a9fc378

Refactor on review

25a674a

Remove commented code to compile haddock

8cb6b51

Bump up chromo-driver

77f839f

willbasky force-pushed the willbasky/jsonb branch from 571cd92 to 77f839f Compare September 10, 2019 11:09

neongreen added 4 commits September 10, 2019 16:48

Better benchmark instructions

007283b

Use a different database name for benchmarks

0ee809a

Remove ghc-options

d7295d5

Add a TODO

c051d27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#389 jsonb #390

#389 jsonb #390

willbasky commented Aug 29, 2019 •

edited by neongreen

neongreen Aug 31, 2019

neongreen Aug 31, 2019

neongreen Aug 31, 2019

willbasky Sep 1, 2019

neongreen Aug 31, 2019

neongreen Aug 31, 2019

willbasky Sep 1, 2019 •

edited

neongreen Aug 31, 2019

neongreen Aug 31, 2019

neongreen Aug 31, 2019

neongreen Aug 31, 2019

neongreen Aug 31, 2019

neongreen Sep 2, 2019

neongreen Sep 2, 2019

neongreen Sep 2, 2019

neongreen Sep 2, 2019

neongreen Sep 2, 2019

neongreen Sep 2, 2019

neongreen Sep 2, 2019

		@@ -0,0 +1,49 @@
		-- \| Module contains all stuff to migrate from AcidState to Postgres.

	-- \| Load categories and archives categories from acid state to postgres
	-- \| Load categories and archived categories from acid state to postgres

	catarchivedPostgres <- runTransactionExceptT conn Read $
	catArchivedPostgres <- runTransactionExceptT conn Read $

	sortOn categoryUid catarchivedPostgres ==
	sortOn categoryUid catArchivedPostgres ==

#389 jsonb #390

Are you sure you want to change the base?

#389 jsonb #390

Conversation

willbasky commented Aug 29, 2019 • edited by neongreen

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willbasky Sep 1, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willbasky commented Aug 29, 2019 •

edited by neongreen

willbasky Sep 1, 2019 •

edited