tests: Skip SQLite fsyncs while testing #21634

achow101 commented at 1:28 AM on April 8, 2021: member

Since we want tests to run quickly, and since tests do a lot more db operations than expected we expect to see in actual usage, we disable sqlite's syncing behavior to make db operations run much faster. This syncing behavior is necessary for normal operation as it helps guarantee that data won't become lost or corrupted, but in tests, we don't care about that.

Fixes #21628

fanquake added the label Tests on Apr 8, 2021

fanquake added the label Wallet on Apr 8, 2021

DrahtBot commented at 4:16 AM on April 8, 2021: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#20773 (refactor: split CWallet::Create by S3RK)
#19873 (Flush dbcache early if system is under memory pressure by luke-jr)
#19101 (refactor: remove ::vpwallets and related global variables by ryanofsky)
#18554 (wallet: ensure wallet files are not reused across chains by rodentrabies)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

in src/wallet/sqlite.cpp:238 in 89a0728d99 outdated

 232 | @@ -233,6 +233,15 @@ void SQLiteDatabase::Open()
 233 |          throw std::runtime_error(strprintf("SQLiteDatabase: Failed to enable fullfsync: %s\n", sqlite3_errstr(ret)));
 234 |      }
 235 |  
 236 | +    if (gArgs.GetBoolArg("-unsafesqlitesync", false)) {
 237 | +        // Use normal synchronous mode for the journal
 238 | +        LogPrintf("SQLiteDatabase: WARNING SQLite is configured to not wait for data to be flushed to disk. Data loss and corruption may occur.");

MarcoFalke commented at 7:54 AM on April 8, 2021:

        LogPrintf("WARNING SQLite is configured to not wait for data to be flushed to disk. Data loss and corruption may occur.\n");

function names and file paths are already logged in tests. Also missing \n to make linter happy?

achow101 commented at 4:37 PM on April 8, 2021:

Done

MarcoFalke commented at 7:55 AM on April 8, 2021: member

Left a linter comment, didn't review

achow101 force-pushed on Apr 8, 2021

in test/functional/test_framework/test_node.py:107 in 7140ed3323 outdated

 102 | @@ -103,6 +103,8 @@ def __init__(self, i, datadir, *, chain, rpchost, timewait, timeout_factor, bitc
 103 |              "-debugexclude=leveldb",
 104 |              "-uacomment=testnode%d" % i,
 105 |          ]
 106 | +        # To improve SQLite wallet performance so that the tests don't timeout, use -unsafesqlitesync
 107 | +        append_config(datadir, ["unsafesqlitesync=1"])

vasild commented at 3:40 PM on April 9, 2021:

Why not add -unsafesqlitesync=1 to self.args[] which is defined just above?

        self.args = [
            self.binary,
            "-datadir=" + self.datadir,
            "-logtimemicros",
            "-debug",
            "-debugexclude=libevent",
            "-debugexclude=leveldb",
            "-uacomment=testnode%d" % i,
+           "-unsafesqlitesync=1",
        ]

achow101 commented at 5:17 PM on April 9, 2021:

self.args is command line only, not in the conf file. I wanted this to be in the bitcoin.conf file, although thinking on that, I'm not sure if it is necessary.

vasild commented at 10:33 AM on April 12, 2021:

I think this question will rise every time somebody looks here (even after merge). Maybe move it to self.args[], or if there is a reason to use append_config(), then mention that reason in a comment.

MarcoFalke commented at 12:31 PM on April 12, 2021:

It is possible to modify write_config if the goal is to add a static option to the conf file

achow101 commented at 11:33 PM on April 12, 2021:

write_config was the function I was looking for. Changed to use that instead.

in src/wallet/init.cpp:86 in 7140ed3323 outdated

  81 | @@ -82,6 +82,12 @@ void WalletInit::AddWalletOptions(ArgsManager& argsman) const
  82 |      argsman.AddHiddenArgs({"-dblogsize", "-flushwallet", "-privdb"});
  83 |  #endif
  84 |  
  85 | +#ifdef USE_SQLITE
  86 | +    argsman.AddArg("-unsafesqlitesync", "Set SQLite synchronous=OFF to disable waiting for the database to sync to disk. This is unsafe and can cause data loss and corruption. This option is only used by tests to improve their performance (default: false)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::WALLET_DEBUG_TEST);

vasild commented at 4:00 PM on April 9, 2021:

Since this is a boolean argument, why not s/ArgsManager::ALLOW_ANY/ArgsManager::ALLOW_BOOL/

achow101 commented at 5:18 PM on April 9, 2021:

Done

vasild commented at 4:06 PM on April 9, 2021: member

How much performance boost does this bring?

achow101 force-pushed on Apr 9, 2021

vasild approved

vasild commented at 10:31 AM on April 12, 2021: member

ACK 5c72335f0f848895079913e58534ac9488c18ed7

Locally,wallet_tests/CreateWallet test time [seconds]:

master (cb79cabd) sqlite: 63, 62, 60
master (cb79cabd) bdb:     2,  3,  3
PR     (5c72335f) sqlite:  3,  3,  3

Maybe add -unsafesqlitesync=1 to self.args[] if there is no reason to use a separate call to append_config().

laanwj commented at 12:17 PM on April 12, 2021: member

Concept ACK. Syncing is a robustness measure, and unless you're testing the syncing itself, there is no use to have it enabled in tests (at the expense if extra testing time).

Seperately it would make sense to look into the sqlite performance issues, but this is a win.

tests: Skip SQLite fsyncs while testing

Since we want tests to run quickly, and since tests do a lot more db
operations than expected we expect to see in actual usage, we disable
sqlite's syncing behavior to make db operations run much faster. This
syncing behavior is necessary for normal operation as it helps guarantee
that data won't become lost or corrupted, but in tests, we don't care
about that.

41f891da50

achow101 force-pushed on Apr 12, 2021

vasild approved

vasild commented at 8:10 AM on April 13, 2021: member

ACK 41f891da508114f1fd4df30b4068073ec30abc2a

MarcoFalke merged this on Apr 13, 2021

MarcoFalke closed this on Apr 13, 2021

sidhujag referenced this in commit c0ef5fd1e1 on Apr 13, 2021

DrahtBot locked this on Aug 18, 2022