test file-based db read/write operations #345

paweljakubas · 2019-05-31T05:02:25Z

Issue Number

Overview

I have added unit tests that check file-backed sqlite db read/write/clean operation in a number of state changing scenerios
I have added property test that checks if we are in line with in-memory version when using random number of state changing operations in a row
I have exposed Connection in newDBLayer in order to have a possibility to close it. I presume it will be needed in file-backed db instances

Comments

I have an error - need to investigate it

This one only locally (not in CV) - maybe number of available connections is lower in my computer:

$ stack test --ta '-m "random operation"' cardano-wallet-core:test:unit

       uncaught exception: SqliteException
       SQLite3 returned ErrorCan'tOpen while attempting to perform open "backup/test.db".
       (after 99 tests and 1 shrink)
         KeyValPairs []

but not really confident here as I have :

[pawel@arch cardano-wallet]$ cat /proc/sys/fs/file-max
1628981

This one is both in my computer and CI:

opening and closing of db works FAILED [1]
     test/unit/Cardano/Wallet/DB/SqliteCorruptionSpec.hs:88:9: 
      1) Cardano.Wallet.DB.SqliteCorruption, Check db opening/closing, opening and closing of db works
           uncaught exception: SqliteException
           SQLite3 returned ErrorBusy while attempting to perform close: unable to close due to unfinalized statements or unfinished backups

rvl

It looks like a worthy thing to test a little, but not too much.

In the QSM tests I originally had an Open command for testing this type of thing. However I removed it because I confirmed that the wallet process will only ever have a single DBLayer instantiated during its lifetime. So we can assume no concurrent access to .db files, and that the very well tested SQLite library will handle "power cable unplugged" type situations.

rvl · 2019-06-03T05:57:06Z

lib/core/src/Cardano/Wallet/DB/Sqlite.hs

@@ -177,17 +178,17 @@ newDBLayer
    :: forall s t. (W.IsOurs s, NFData s, Show s, PersistState s, W.TxId t)
    => Maybe FilePath
       -- ^ Database file location, or Nothing for in-memory database
-    -> IO (DBLayer IO s t)
+    -> IO (Sqlite.Connection, DBLayer IO s t)
 newDBLayer fp = do


Perhaps call this newDBLayer' and have newDBLayer = fmap snd . newDBLayer'?

rvl · 2019-06-03T06:01:46Z

lib/core/backup/.gitignore

+# Ignore everything in this directory
+*
+# Except this file
+!.gitignore


I reckon it will be better for tests to create a new directory in /tmp so that they're less susceptible to differences in local state.

rvl · 2019-06-03T06:12:44Z

lib/core/test/unit/Cardano/Wallet/DB/SqliteCorruptionSpec.hs

+            close conn
+
+
+prop_randomOpChunks


Suggested change

prop_randomOpChunks

-- This property checks that executing series of wallet operations in a single

-- SQLite session has the same effect as executing the same operations over

-- multiple sessions.

prop_randomOpChunks

rvl · 2019-06-03T06:31:04Z

lib/core/test/unit/Cardano/Wallet/DB/SqliteCorruptionSpec.hs

+            unsafeRunExceptT $ createWallet db testPk testCp testMetadata
+            listWallets db `shouldReturn` [testPk]
+            close conn
+            forM_ [1..25] openCloseDB


Would replicateM_ 25 openCloseDB be equivalent?

rvl · 2019-06-03T06:34:28Z

lib/core/test/unit/Cardano/Wallet/DB/SqliteCorruptionSpec.hs

+            -> s
+            -> s
+            -> Expectation
+        testOpeningCleaning call expectedAfterOpen expectedAfterClean = do


I suspect that cleanDB is already pretty well tested because it's run before every DBSpec test case.

But I think the intention of this is to check that changes made with one connection have an effect on the db file, which can be seen from subsequent connections.

cleanDB works fine - I agree, but we had problem with closing DB. See comment below for more details on it

rvl · 2019-06-03T06:42:50Z

I'm guessing the SQLite ErrorBusy exception is due to the tests not promptly closing the database connection.

paweljakubas · 2019-06-03T13:32:05Z

@rvl @KtorZ I made not so small investigation. When we have disk-backed db we should probably be able to close gracefully. I added a number of tests to persuade myself and others that this is the case. A number of things popped out:
Even if we only use newDBLayer and then want to close it we were not able to do it.
This was due to :

enableForeignKeys :: Sqlite.Connection -> IO ()
enableForeignKeys conn = stmt >>= void . Sqlite.step
    where stmt = Sqlite.prepare conn "PRAGMA foreign_keys = ON;"

Sqlite.finalize stmt was missing here.

Moving forward, I was not able to just migrate and addIndexes. I dived deeper and this was probably because of not using withSqlConn in the current version of runQuery' . When using it, close' from Data.Persist.Sql is used and backend is closed afterwards (see implementation). After adopting new version of runQuery' with withSqlConn open/closing roundtrip tests started to pass. This refers to file-based solution.

Still createSqlBackend needs refactoring and we have one property test failing (although open/close tests and unit tests passes) :

Migrating: CREATE TABLE "seq_state_pending_ix"("seq_state_pending_ix_seq_state_id" INTEGER NOT NULL REFERENCES "seq_state","seq_state_pending_ix_index" INTEGER NOT NULL, PRIMARY KEY ("seq_state_pending_ix_seq_state_id","seq_state_pending_ix_index"))
    realize a random batch of operations upon one db open FAILED [1]

Failures:

  test/unit/Cardano/Wallet/DB/SqliteCorruptionSpec.hs:160:9: 
  1) Cardano.Wallet.DB.SqliteCorruption, random operation chunks property when writing to/reading from file, realize a random batch of operations upon one db open
       uncaught exception: SqliteException
       SQLite3 returned ErrorError while attempting to perform prepare "CREATE INDEX IF NOT EXISTS pending_tx_wallet_id ON pending_tx (wallet_id)": no such table: main.pending_tx
       (after 1 test)

In detail we have in CI (the same reason as above listed ):

    Cardano.Wallet.DB.SqliteCorruption
      Check db opening/closing
        opening and closing of db works
      Check db reading/writing from/to file and cleaning
        create and list wallet works
        create and get meta works
        create and get private key
        put and read tx history
        put and read checkpoint
      random operation chunks property when writing to/reading from file
        realize a random batch of operations upon one db open FAILED [1]
    Cardano.Wallet.DB.Sqlite
      Sqlite Simple tests
        Wallet table
          create and list works FAILED [2]
          create and get meta works

I believe, this kind of testing is important as it is exposing some not so obvious issues that could come up with db connection and would be very difficult to understand later.

paweljakubas · 2019-06-03T13:33:29Z

lib/core/src/Cardano/Wallet/DB/Sqlite.hs

+enableForeignKeys conn = do
+    stmt <- Sqlite.prepare conn "PRAGMA foreign_keys = ON;"
+    _ <- Sqlite.step stmt
+    Sqlite.finalize stmt


this is needed to be able to close and avoid SQLite3 returned ErrorBusy while attempting to perform close: unable to close due to unfinalized statements or unfinished backups

paweljakubas · 2019-06-03T13:34:15Z

lib/core/src/Cardano/Wallet/DB/Sqlite.hs

+    backend <- wrapConnection conn logFunc
+    pure (conn, backend)
+
+createSqliteBackend1 :: Maybe FilePath -> LogFunc -> IO SqlBackend


need still to think how this can be married with createSqliteBackend

paweljakubas · 2019-06-03T13:34:51Z

lib/core/src/Cardano/Wallet/DB/Sqlite.hs

+    (conn, _backend) <- createSqliteBackend fp (dbLogs [LevelError])
+
+    let runQuery' cmd =
+            withMVar bigLock $ const $ runResourceT $ runNoLoggingT $ withSqlConn (createSqliteBackend1 fp)


using withSqlConn is needed to be able to close and avoid SQLite3 returned ErrorBusy while attempting to perform close: unable to close due to unfinalized statements or unfinished backups OR we can somehow use close' there (remarks refers to file-based solution)

paweljakubas · 2019-06-03T17:46:35Z

Strange:

newDBLayer Nothing

gives uncaught exception: SqliteException SQLite3 returned ErrorError while attempting to perform prepare "CREATE INDEX IF NOT EXISTS pending_tx_wallet_id ON pending_tx (wallet_id)": no such table: main.pending_tx

In contrast to

newDBLayer (Just "anything")

which works fine.
Logs point to addIndexes

paweljakubas · 2019-06-03T18:08:36Z

@rvl @KtorZ I found the way to be green in this ^^^ hope you will examine this and better unify what I found - in the end it turn out that for in-memory mode there is old way (modulo finalize improvement in enableForeignKeys) and for file-based solution the one I proposed earlier.

rvl · 2019-06-04T09:30:20Z

@paweljakubas I look through the tests, poked some things, and tried to make the failing test pass.

However I don't think it's a realistic test to call Database.Sqlite.close on the connection used by DBLayer.

…capability to close it compile fix

hlint fix

paweljakubas · 2019-06-05T20:26:30Z

@KtorZ I see once again my missing commits in the end, I recommend to close this PR in favor of #370 (this one is up-to-date, and by-and-large what was on Monday late evening)

KtorZ · 2019-06-06T11:07:25Z

Closing in favor of #370

paweljakubas requested review from rvl and KtorZ May 31, 2019 05:02

paweljakubas self-assigned this May 31, 2019

akegalj mentioned this pull request May 31, 2019

Extend the current DB layer with an SQLite interface #154

Closed

12 tasks

paweljakubas force-pushed the paweljakubas/154/add-db-read-write-to-file branch 3 times, most recently from 15ed9d9 to 19bb208 Compare May 31, 2019 11:16

rvl reviewed Jun 3, 2019

View reviewed changes

paweljakubas force-pushed the paweljakubas/154/add-db-read-write-to-file branch from 19bb208 to fd3ca18 Compare June 3, 2019 13:22

paweljakubas commented Jun 3, 2019

View reviewed changes

rvl force-pushed the paweljakubas/154/add-db-read-write-to-file branch from 7b75243 to 87e8346 Compare June 4, 2019 06:39

rvl force-pushed the paweljakubas/154/add-db-read-write-to-file branch from f4a6d02 to 7b75243 Compare June 5, 2019 10:46

paweljakubas and others added 7 commits June 5, 2019 17:14

Add unit tests

c200156

Extend one createWallet unit test

92b88cf

add step-by-step inmemory vs file db comparison property

69aa061

add random batch to property and extract connection in order to have …

7632cf4

…capability to close it compile fix

add additional open/close db test

027424e

hlint fix

wip: Rebase and clean up tests

158b031

wip: Clean up

71ebcd8

KtorZ force-pushed the paweljakubas/154/add-db-read-write-to-file branch from 7b75243 to 71ebcd8 Compare June 5, 2019 15:15

KtorZ closed this Jun 6, 2019

KtorZ deleted the paweljakubas/154/add-db-read-write-to-file branch June 6, 2019 11:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test file-based db read/write operations #345

test file-based db read/write operations #345

paweljakubas commented May 31, 2019 •

edited

Loading

rvl left a comment

rvl Jun 3, 2019

rvl Jun 3, 2019

rvl Jun 3, 2019

rvl Jun 3, 2019

rvl Jun 3, 2019

paweljakubas Jun 3, 2019

rvl commented Jun 3, 2019

paweljakubas commented Jun 3, 2019 •

edited

Loading

paweljakubas Jun 3, 2019

paweljakubas Jun 3, 2019

paweljakubas Jun 3, 2019 •

edited

Loading

paweljakubas commented Jun 3, 2019 •

edited

Loading

paweljakubas commented Jun 3, 2019 •

edited

Loading

rvl commented Jun 4, 2019

paweljakubas commented Jun 5, 2019

KtorZ commented Jun 6, 2019

-prop_randomOpChunks
+-- This property checks that executing series of wallet operations in a single
+-- SQLite session has the same effect as executing the same operations over
+-- multiple sessions.
+prop_randomOpChunks

test file-based db read/write operations #345

test file-based db read/write operations #345

Conversation

paweljakubas commented May 31, 2019 • edited Loading

Issue Number

Overview

Comments

rvl left a 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

rvl commented Jun 3, 2019

paweljakubas commented Jun 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paweljakubas Jun 3, 2019 • edited Loading

Choose a reason for hiding this comment

paweljakubas commented Jun 3, 2019 • edited Loading

paweljakubas commented Jun 3, 2019 • edited Loading

rvl commented Jun 4, 2019

paweljakubas commented Jun 5, 2019

KtorZ commented Jun 6, 2019

paweljakubas commented May 31, 2019 •

edited

Loading

paweljakubas commented Jun 3, 2019 •

edited

Loading

paweljakubas Jun 3, 2019 •

edited

Loading

paweljakubas commented Jun 3, 2019 •

edited

Loading

paweljakubas commented Jun 3, 2019 •

edited

Loading