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
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
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.
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.");
0 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?
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"])
Why not add -unsafesqlitesync=1 to self.args[] which is defined just above?
0 self.args = [
1 self.binary,
2 "-datadir=" + self.datadir,
3 "-logtimemicros",
4 "-debug",
5 "-debugexclude=libevent",
6 "-debugexclude=leveldb",
7 "-uacomment=testnode%d" % i,
8+ "-unsafesqlitesync=1",
9 ]
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.
self.args[], or if there is a reason to use append_config(), then mention that reason in a comment.
write_config if the goal is to add a static option to the conf file
write_config was the function I was looking for. Changed to use that instead.
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);
s/ArgsManager::ALLOW_ANY/ArgsManager::ALLOW_BOOL/
ACK 5c72335f0f848895079913e58534ac9488c18ed7
Locally,wallet_tests/CreateWallet test time [seconds]:
0master (cb79cabd) sqlite: 63, 62, 60
1master (cb79cabd) bdb: 2, 3, 3
2PR (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().
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.
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.