From 7a198bba0a1d0a0f0fd4ca947955cb52b84bdd4b Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 11 Apr 2022 15:14:24 -0400 Subject: [PATCH] wallet: Introduce DatabaseCursor RAII class for managing cursor Instead of having DatabaseBatch deal with opening and closing database cursors, have a separate RAII class that deals with those. For now, DatabaseBatch manages DatabaseCursor, but this will change later. --- src/wallet/bdb.cpp | 27 ++++++++++++++------- src/wallet/bdb.h | 17 +++++++++---- src/wallet/db.h | 41 +++++++++++++++++++++++++++----- src/wallet/sqlite.cpp | 39 +++++++++++++++++------------- src/wallet/sqlite.h | 18 +++++++++----- src/wallet/test/wallet_tests.cpp | 10 +++++--- 6 files changed, 108 insertions(+), 44 deletions(-) diff --git a/src/wallet/bdb.cpp b/src/wallet/bdb.cpp index d45e8679ac..edfd443b7d 100644 --- a/src/wallet/bdb.cpp +++ b/src/wallet/bdb.cpp @@ -8,6 +8,7 @@ #include #include +#include #include #include @@ -307,7 +308,7 @@ BerkeleyDatabase::~BerkeleyDatabase() } } -BerkeleyBatch::BerkeleyBatch(BerkeleyDatabase& database, const bool read_only, bool fFlushOnCloseIn) : pdb(nullptr), activeTxn(nullptr), m_cursor(nullptr), m_database(database) +BerkeleyBatch::BerkeleyBatch(BerkeleyDatabase& database, const bool read_only, bool fFlushOnCloseIn) : pdb(nullptr), activeTxn(nullptr), m_database(database) { database.AddRef(); database.Open(); @@ -656,16 +657,18 @@ void BerkeleyDatabase::ReloadDbEnv() env->ReloadDbEnv(); } -bool BerkeleyBatch::StartCursor() +BerkeleyCursor::BerkeleyCursor(BerkeleyDatabase& database) { - assert(!m_cursor); - if (!pdb) - return false; - int ret = pdb->cursor(nullptr, &m_cursor, 0); - return ret == 0; + if (!database.m_db.get()) { + throw std::runtime_error(STR_INTERNAL_BUG("BerkeleyDatabase does not exist")); + } + int ret = database.m_db->cursor(nullptr, &m_cursor, 0); + if (ret != 0) { + throw std::runtime_error(STR_INTERNAL_BUG(strprintf("BDB Cursor could not be created. Returned %d", ret).c_str())); + } } -bool BerkeleyBatch::ReadAtCursor(CDataStream& ssKey, CDataStream& ssValue, bool& complete) +bool BerkeleyCursor::Next(CDataStream& ssKey, CDataStream& ssValue, bool& complete) { complete = false; if (m_cursor == nullptr) return false; @@ -691,13 +694,19 @@ bool BerkeleyBatch::ReadAtCursor(CDataStream& ssKey, CDataStream& ssValue, bool& return true; } -void BerkeleyBatch::CloseCursor() +BerkeleyCursor::~BerkeleyCursor() { if (!m_cursor) return; m_cursor->close(); m_cursor = nullptr; } +std::unique_ptr BerkeleyBatch::GetNewCursor() +{ + if (!pdb) return nullptr; + return std::make_unique(m_database); +} + bool BerkeleyBatch::TxnBegin() { if (!pdb || activeTxn) diff --git a/src/wallet/bdb.h b/src/wallet/bdb.h index 35cc823876..0ee94fa2bc 100644 --- a/src/wallet/bdb.h +++ b/src/wallet/bdb.h @@ -185,6 +185,18 @@ public: operator Dbt*(); }; +class BerkeleyCursor : public DatabaseCursor +{ +private: + Dbc* m_cursor; + +public: + explicit BerkeleyCursor(BerkeleyDatabase& database); + ~BerkeleyCursor() override; + + bool Next(CDataStream& key, CDataStream& value, bool& complete) override; +}; + /** RAII class that provides access to a Berkeley database */ class BerkeleyBatch : public DatabaseBatch { @@ -198,7 +210,6 @@ protected: Db* pdb; std::string strFile; DbTxn* activeTxn; - Dbc* m_cursor; bool fReadOnly; bool fFlushOnClose; BerkeleyEnvironment *env; @@ -214,9 +225,7 @@ public: void Flush() override; void Close() override; - bool StartCursor() override; - bool ReadAtCursor(CDataStream& ssKey, CDataStream& ssValue, bool& complete) override; - void CloseCursor() override; + std::unique_ptr GetNewCursor() override; bool TxnBegin() override; bool TxnCommit() override; bool TxnAbort() override; diff --git a/src/wallet/db.h b/src/wallet/db.h index f09844c37e..aa1377ccef 100644 --- a/src/wallet/db.h +++ b/src/wallet/db.h @@ -22,10 +22,24 @@ struct bilingual_str; namespace wallet { void SplitWalletPath(const fs::path& wallet_path, fs::path& env_directory, std::string& database_filename); +class DatabaseCursor +{ +public: + explicit DatabaseCursor() {} + virtual ~DatabaseCursor() {} + + DatabaseCursor(const DatabaseCursor&) = delete; + DatabaseCursor& operator=(const DatabaseCursor&) = delete; + + virtual bool Next(CDataStream& key, CDataStream& value, bool& complete) { return false; } +}; + /** RAII class that provides access to a WalletDatabase */ class DatabaseBatch { private: + std::unique_ptr m_cursor; + virtual bool ReadKey(CDataStream&& key, CDataStream& value) = 0; virtual bool WriteKey(CDataStream&& key, CDataStream&& value, bool overwrite=true) = 0; virtual bool EraseKey(CDataStream&& key) = 0; @@ -92,9 +106,21 @@ public: return HasKey(std::move(ssKey)); } - virtual bool StartCursor() = 0; - virtual bool ReadAtCursor(CDataStream& ssKey, CDataStream& ssValue, bool& complete) = 0; - virtual void CloseCursor() = 0; + virtual std::unique_ptr GetNewCursor() = 0; + bool StartCursor() + { + m_cursor = GetNewCursor(); + return m_cursor != nullptr; + } + bool ReadAtCursor(CDataStream& ssKey, CDataStream& ssValue, bool& complete) + { + if (!m_cursor) return false; + return m_cursor->Next(ssKey, ssValue, complete); + } + void CloseCursor() + { + m_cursor.reset(); + } virtual bool TxnBegin() = 0; virtual bool TxnCommit() = 0; virtual bool TxnAbort() = 0; @@ -156,6 +182,11 @@ public: virtual std::unique_ptr MakeBatch(bool flush_on_close = true) = 0; }; +class DummyCursor : public DatabaseCursor +{ + bool Next(CDataStream& key, CDataStream& value, bool& complete) override { return false; } +}; + /** RAII class that provides access to a DummyDatabase. Never fails. */ class DummyBatch : public DatabaseBatch { @@ -169,9 +200,7 @@ public: void Flush() override {} void Close() override {} - bool StartCursor() override { return true; } - bool ReadAtCursor(CDataStream& ssKey, CDataStream& ssValue, bool& complete) override { return true; } - void CloseCursor() override {} + std::unique_ptr GetNewCursor() override { return std::make_unique(); } bool TxnBegin() override { return true; } bool TxnCommit() override { return true; } bool TxnAbort() override { return true; } diff --git a/src/wallet/sqlite.cpp b/src/wallet/sqlite.cpp index 053fb8f983..bc1b524adc 100644 --- a/src/wallet/sqlite.cpp +++ b/src/wallet/sqlite.cpp @@ -125,7 +125,6 @@ void SQLiteBatch::SetupSQLStatements() {&m_insert_stmt, "INSERT INTO main VALUES(?, ?)"}, {&m_overwrite_stmt, "INSERT or REPLACE into main values(?, ?)"}, {&m_delete_stmt, "DELETE FROM main WHERE key = ?"}, - {&m_cursor_stmt, "SELECT key, value FROM main"}, }; for (const auto& [stmt_prepared, stmt_text] : statements) { @@ -374,7 +373,6 @@ void SQLiteBatch::Close() {&m_insert_stmt, "insert"}, {&m_overwrite_stmt, "overwrite"}, {&m_delete_stmt, "delete"}, - {&m_cursor_stmt, "cursor"}, }; for (const auto& [stmt_prepared, stmt_description] : statements) { @@ -472,27 +470,17 @@ bool SQLiteBatch::HasKey(CDataStream&& key) return res == SQLITE_ROW; } -bool SQLiteBatch::StartCursor() -{ - assert(!m_cursor_init); - if (!m_database.m_db) return false; - m_cursor_init = true; - return true; -} - -bool SQLiteBatch::ReadAtCursor(CDataStream& key, CDataStream& value, bool& complete) +bool SQLiteCursor::Next(CDataStream& key, CDataStream& value, bool& complete) { complete = false; - if (!m_cursor_init) return false; - int res = sqlite3_step(m_cursor_stmt); if (res == SQLITE_DONE) { complete = true; return true; } if (res != SQLITE_ROW) { - LogPrintf("SQLiteBatch::ReadAtCursor: Unable to execute cursor step: %s\n", sqlite3_errstr(res)); + LogPrintf("%s: Unable to execute cursor step: %s\n", __func__, sqlite3_errstr(res)); return false; } @@ -506,10 +494,29 @@ bool SQLiteBatch::ReadAtCursor(CDataStream& key, CDataStream& value, bool& compl return true; } -void SQLiteBatch::CloseCursor() +SQLiteCursor::~SQLiteCursor() { sqlite3_reset(m_cursor_stmt); - m_cursor_init = false; + int res = sqlite3_finalize(m_cursor_stmt); + if (res != SQLITE_OK) { + LogPrintf("%s: cursor closed but could not finalize cursor statement: %s\n", + __func__, sqlite3_errstr(res)); + } +} + +std::unique_ptr SQLiteBatch::GetNewCursor() +{ + if (!m_database.m_db) return nullptr; + auto cursor = std::make_unique(); + + const char* stmt_text = "SELECT key, value FROM main"; + int res = sqlite3_prepare_v2(m_database.m_db, stmt_text, -1, &cursor->m_cursor_stmt, nullptr); + if (res != SQLITE_OK) { + throw std::runtime_error(strprintf( + "%s: Failed to setup cursor SQL statement: %s\n", __func__, sqlite3_errstr(res))); + } + + return cursor; } bool SQLiteBatch::TxnBegin() diff --git a/src/wallet/sqlite.h b/src/wallet/sqlite.h index 47b7ebb0ec..286ee1613d 100644 --- a/src/wallet/sqlite.h +++ b/src/wallet/sqlite.h @@ -14,19 +14,27 @@ struct bilingual_str; namespace wallet { class SQLiteDatabase; +class SQLiteCursor : public DatabaseCursor +{ +public: + sqlite3_stmt* m_cursor_stmt{nullptr}; + + explicit SQLiteCursor() {} + ~SQLiteCursor() override; + + bool Next(CDataStream& key, CDataStream& value, bool& complete) override; +}; + /** RAII class that provides access to a WalletDatabase */ class SQLiteBatch : public DatabaseBatch { private: SQLiteDatabase& m_database; - bool m_cursor_init = false; - sqlite3_stmt* m_read_stmt{nullptr}; sqlite3_stmt* m_insert_stmt{nullptr}; sqlite3_stmt* m_overwrite_stmt{nullptr}; sqlite3_stmt* m_delete_stmt{nullptr}; - sqlite3_stmt* m_cursor_stmt{nullptr}; void SetupSQLStatements(); @@ -44,9 +52,7 @@ public: void Close() override; - bool StartCursor() override; - bool ReadAtCursor(CDataStream& key, CDataStream& value, bool& complete) override; - void CloseCursor() override; + std::unique_ptr GetNewCursor() override; bool TxnBegin() override; bool TxnCommit() override; bool TxnAbort() override; diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 0f703b7d00..ea4dd30dea 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -867,6 +867,12 @@ BOOST_FIXTURE_TEST_CASE(ZapSelectTx, TestChain100Setup) TestUnloadWallet(std::move(wallet)); } +class FailCursor : public DatabaseCursor +{ +public: + bool Next(CDataStream& key, CDataStream& value, bool& complete) override { return false; } +}; + /** RAII class that provides access to a FailDatabase. Which fails if needed. */ class FailBatch : public DatabaseBatch { @@ -882,9 +888,7 @@ public: void Flush() override {} void Close() override {} - bool StartCursor() override { return true; } - bool ReadAtCursor(CDataStream& ssKey, CDataStream& ssValue, bool& complete) override { return false; } - void CloseCursor() override {} + std::unique_ptr GetNewCursor() override { return std::make_unique(); } bool TxnBegin() override { return false; } bool TxnCommit() override { return false; } bool TxnAbort() override { return false; }