wallet: BerkeleyBatch Handle cursor internally #19308

achow101 · 2020-06-17T16:41:54Z

Instead of returning a Dbc (BDB cursor object) and having the caller deal with the cursor, make BerkeleyBatch handle the cursor internally.

Split from #18971

DrahtBot · 2020-06-17T23:18:08Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

wallet: Cleanup and separate BerkeleyDatabase and BerkeleyBatch #19335 (wallet: Cleanup and separate BerkeleyDatabase and BerkeleyBatch by achow101)
wallet: Introduce WalletDatabase abstract class #19334 (wallet: Introduce WalletDatabase abstract class by achow101)
wallet: Refactor BerkeleyDatabase to introduce DatabaseBatch abstract class #19325 (wallet: Refactor BerkeleyDatabase to introduce DatabaseBatch abstract class by achow101)
wallettool: Add dump and createfromdump commands #19137 (wallettool: Add dump and createfromdump commands by achow101)
wallet: Introduce and use DummyDatabase instead of dummy BerkeleyDatabase #19102 (wallet: Introduce and use DummyDatabase instead of dummy BerkeleyDatabase by achow101)
refactor: Remove CAddressBookData::destdata #18608 (refactor: Remove CAddressBookData::destdata by ryanofsky)

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.

src/wallet/walletdb.cpp

laanwj · 2020-06-18T14:49:25Z

This does limit flexibility a bit, though I guess one cursor at a time is enough for all our current and future use cases.

src/wallet/bdb.cpp

achow101 · 2020-06-18T14:58:28Z

This does limit flexibility a bit, though I guess one cursor at a time is enough for all our current and future use cases.

If another cursor was needed, you could use another Batch object. But we barely use cursors, so one is enough for now.

laanwj · 2020-06-18T15:03:59Z

If another cursor was needed, you could use another Batch object. But we barely use cursors, so one is enough for now.

Ohh! I missed that it's on the batch, not on the database object.

ryanofsky

Code review ACK cda86544ae9de430e4558b87945046d5612202ac. Nice to make cursor usage safer & more generic, even if it's a slightly less flexible than before (limited to single cursor per batch). I left minor style suggestion below, but feel free to ignore

src/wallet/walletdb.cpp

promag

Concept ACK.

src/wallet/bdb.cpp

src/wallet/walletdb.cpp

Instead of returning a Dbc (BDB cursor object) and having the caller deal with the cursor, make BerkeleyBatch handle the cursor internally. This prepares BerkeleyBatch to work with other database systems as Dbc objects are BDB specific.

promag · 2020-06-22T21:33:26Z

src/wallet/bdb.cpp

+bool BerkeleyBatch::ReadAtCursor(CDataStream& ssKey, CDataStream& ssValue, bool& complete)
 {
+    complete = false;
+    if (m_cursor == nullptr) return false;


assert(m_cursor)? There was no check for pcursor.

I don't think an assert is necessary.

If it's something that can't happen then an assertion is fine. Also, it means that StartCursor must be called before.

If I have to push again.

promag · 2020-06-22T21:33:43Z

src/wallet/bdb.cpp



-BerkeleyBatch::BerkeleyBatch(BerkeleyDatabase& database, const char* pszMode, bool fFlushOnCloseIn) : pdb(nullptr), activeTxn(nullptr)
+BerkeleyBatch::BerkeleyBatch(BerkeleyDatabase& database, const char* pszMode, bool fFlushOnCloseIn) : pdb(nullptr), activeTxn(nullptr), m_cursor(nullptr)


nit, move these to header.

I'm pretty sure that won't compile.

I mean 04998ef.

Ah. Maybe if I have to push again.

ryanofsky

Code review ACK ca24edf. Changes since last review: StartCursor rename, moving CloseCursor calls near returns

promag

Code review ACK ca24edf.

promag · 2020-06-23T09:31:16Z

src/wallet/bdb.cpp

+bool BerkeleyBatch::ReadAtCursor(CDataStream& ssKey, CDataStream& ssValue, bool& complete)
 {
+    complete = false;
+    if (m_cursor == nullptr) return false;


If it's something that can't happen then an assertion is fine. Also, it means that StartCursor must be called before.

…Batch abstract class b82f0ca walletdb: Add MakeBatch function to BerkeleyDatabase and use it (Andrew Chow) eac9200 walletdb: Refactor DatabaseBatch abstract class from BerkeleyBatch (Andrew Chow) Pull request description: In order to support alternative database systems, we need to have a generic `Batch` class. This PR adds a `DatabaseBatch` abstract class which is implemented by `BerkeleyBatch`. `DatabaseBatch` is now the class that is used by `WalletBatch` to interact with the database. To be able to get the correct type of `DatabaseBatch`, `BerkeleyDatabase` now has a `MakeBatch` function which returns a newly constructed `std::unique_ptr<DatabaseBatch>`. For `BerkeleyDatabase`, that will be `std::unique_ptr<BerkeleyBatch>`. The `Read`, `Write`, `Erase`, and `Exists` template functions are moved from `BerkeleyBatch`. Part of #18971 Requires #19308 and #19324 ACKs for top commit: Sjors: re-utACK b82f0ca MarcoFalke: ACK b82f0ca 🌘 meshcollider: LGTM, utACK b82f0ca Tree-SHA512: 6d2d41631c0983391dbecd702e881c6775b155c90b275df97f7157e42608ed251744f9d7ce5173d02a6c5cc38d90b611880fac7fa635d3d8c4d590681f56ac6a

…atabaseBatch abstract class b82f0ca walletdb: Add MakeBatch function to BerkeleyDatabase and use it (Andrew Chow) eac9200 walletdb: Refactor DatabaseBatch abstract class from BerkeleyBatch (Andrew Chow) Pull request description: In order to support alternative database systems, we need to have a generic `Batch` class. This PR adds a `DatabaseBatch` abstract class which is implemented by `BerkeleyBatch`. `DatabaseBatch` is now the class that is used by `WalletBatch` to interact with the database. To be able to get the correct type of `DatabaseBatch`, `BerkeleyDatabase` now has a `MakeBatch` function which returns a newly constructed `std::unique_ptr<DatabaseBatch>`. For `BerkeleyDatabase`, that will be `std::unique_ptr<BerkeleyBatch>`. The `Read`, `Write`, `Erase`, and `Exists` template functions are moved from `BerkeleyBatch`. Part of bitcoin#18971 Requires bitcoin#19308 and bitcoin#19324 ACKs for top commit: Sjors: re-utACK b82f0ca MarcoFalke: ACK b82f0ca 🌘 meshcollider: LGTM, utACK b82f0ca Tree-SHA512: 6d2d41631c0983391dbecd702e881c6775b155c90b275df97f7157e42608ed251744f9d7ce5173d02a6c5cc38d90b611880fac7fa635d3d8c4d590681f56ac6a

Summary: Instead of returning a Dbc (BDB cursor object) and having the caller deal with the cursor, make BerkeleyBatch handle the cursor internally. This prepares BerkeleyBatch to work with other database systems as Dbc objects are BDB specific. This is a backport of [[bitcoin/bitcoin#19308 | core#19308]] Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Subscribers: majcosta Differential Revision: https://reviews.bitcoinabc.org/D9969

refs: [bitcoin#19290](bitcoin/bitcoin#19290) - Move BDB specific things into bdb.{cpp/h} refs: [bitcoin#19292](bitcoin/bitcoin#19292) - refactor Read, Write, Erase, and Exists into non-template functions refs: [bitcoin#19308](bitcoin/bitcoin#19308) - BerkeleyBatch Handle cursor internally refs: [bitcoin#19324](bitcoin/bitcoin#19324) - Move BerkeleyBatch static functions to BerkeleyDatabase refs: [bitcoin#19320](bitcoin/bitcoin#19320) - Replace CDataStream& with CDataStream&& where appropriate refs: [bitcoin#19325](bitcoin/bitcoin#19325) - Refactor BerkeleyDatabase to introduce DatabaseBatch abstract class refs: [bitcoin#19334](bitcoin/bitcoin#19334) - Introduce WalletDatabase abstract class refs: [bitcoin#19102](bitcoin/bitcoin#19102) - Introduce and use DummyDatabase instead of dummy BerkeleyDatabase refs: [bitcoin#19085](bitcoin/bitcoin#19085) - clean-up PeriodicFlush() refs: [bitcoin#18923](bitcoin/bitcoin#18923) - Never schedule MaybeCompactWalletDB when -flushwallet is off refs: [bitcoin#19335](bitcoin/bitcoin#19335) - Cleanup and separate BerkeleyDatabase and BerkeleyBatch drop salvage wallet from UI remove BDB version from Information Tab refs: [bitcoin#19671](bitcoin/bitcoin#19671) - Remove -zapwallettxes refs: [bitcoin#19261](bitcoin/bitcoin#19261) - Drop ::HasWallets()

achow101 mentioned this pull request Jun 17, 2020

wallet: Refactor the classes in wallet/db.{cpp/h} #18971

Closed

8 tasks

achow101 force-pushed the bdb-internal-cursor branch from 148641d to 40d31e3 Compare June 17, 2020 17:58

DrahtBot added the Wallet label Jun 17, 2020

laanwj reviewed Jun 18, 2020

View reviewed changes

src/wallet/walletdb.cpp Outdated Show resolved Hide resolved

laanwj reviewed Jun 18, 2020

View reviewed changes

src/wallet/bdb.cpp Outdated Show resolved Hide resolved

achow101 force-pushed the bdb-internal-cursor branch from 40d31e3 to cda8654 Compare June 18, 2020 14:57

ryanofsky approved these changes Jun 18, 2020

View reviewed changes

src/wallet/walletdb.cpp Outdated Show resolved Hide resolved

achow101 mentioned this pull request Jun 18, 2020

wallet: Refactor BerkeleyDatabase to introduce DatabaseBatch abstract class #19325

Merged

This was referenced Jun 20, 2020

wallet: Cleanup and separate BerkeleyDatabase and BerkeleyBatch #19335

Merged

wallet: Introduce WalletDatabase abstract class #19334

Merged

promag reviewed Jun 21, 2020

View reviewed changes

src/wallet/bdb.cpp Outdated Show resolved Hide resolved

src/wallet/walletdb.cpp Outdated Show resolved Hide resolved

walletdb: Handle cursor internally

ca24edf

Instead of returning a Dbc (BDB cursor object) and having the caller deal with the cursor, make BerkeleyBatch handle the cursor internally. This prepares BerkeleyBatch to work with other database systems as Dbc objects are BDB specific.

achow101 force-pushed the bdb-internal-cursor branch from cda8654 to ca24edf Compare June 22, 2020 19:43

promag reviewed Jun 22, 2020

View reviewed changes

ryanofsky approved these changes Jun 23, 2020

View reviewed changes

promag reviewed Jun 23, 2020

View reviewed changes

laanwj merged commit 2629174 into bitcoin:master Jul 1, 2020

kwvg added a commit to kwvg/dash that referenced this pull request Mar 6, 2022

merge bitcoin#19308: BerkeleyBatch Handle cursor internally

a276282

bitcoin locked as resolved and limited conversation to collaborators Aug 30, 2022



		BerkeleyBatch::BerkeleyBatch(BerkeleyDatabase& database, const char* pszMode, bool fFlushOnCloseIn) : pdb(nullptr), activeTxn(nullptr)
		BerkeleyBatch::BerkeleyBatch(BerkeleyDatabase& database, const char* pszMode, bool fFlushOnCloseIn) : pdb(nullptr), activeTxn(nullptr), m_cursor(nullptr)

wallet: BerkeleyBatch Handle cursor internally #19308

wallet: BerkeleyBatch Handle cursor internally #19308

Uh oh!

Conversation

achow101 commented Jun 17, 2020

Uh oh!

DrahtBot commented Jun 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Uh oh!

laanwj commented Jun 18, 2020

Uh oh!

Uh oh!

achow101 commented Jun 18, 2020

Uh oh!

laanwj commented Jun 18, 2020

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

DrahtBot commented Jun 17, 2020 •

edited

Loading