wallet: Refactor database cursor into its own object with proper return codes#26690
Merged
fanquake merged 4 commits intobitcoin:masterfrom Jan 23, 2023
Merged
Conversation
Contributor
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, 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. |
This was referenced Dec 12, 2022
achow101
force-pushed
the
walletdb-cursor-unstupidfy
branch
2 times, most recently
from
0d1dade to
576c8fb
Compare
furszy
reviewed
Dec 13, 2022
achow101
force-pushed
the
walletdb-cursor-unstupidfy
branch
from
576c8fb to
115b3ab
Compare
theStack
reviewed
Dec 14, 2022
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.
achow101
force-pushed
the
walletdb-cursor-unstupidfy
branch
from
115b3ab to
bfef559
Compare
This was referenced Dec 16, 2022
Merged
achow101
force-pushed
the
walletdb-cursor-unstupidfy
branch
from
bfef559 to
5e8b838
Compare
Instead of having the DatabaseBatch manage the cursor, having the consumer handle it directly
Next()'s result is a tri-state - failed, more to go, complete. Replace the way that this is returned with an enum with values FAIL, MORE, and DONE rather than with two booleans.
achow101
force-pushed
the
walletdb-cursor-unstupidfy
branch
from
5e8b838 to
4aebd83
Compare
furszy
approved these changes
Dec 16, 2022
kiminuo
reviewed
Dec 29, 2022
| { | ||
| sqlite3_reset(m_cursor_stmt); | ||
| m_cursor_init = false; | ||
| int res = sqlite3_finalize(m_cursor_stmt); |
Contributor
There was a problem hiding this comment.
L496: Why is it necessary to reset the cursor before sqlite3_finalize is called?
Member
Author
There was a problem hiding this comment.
It probably isn't necessary, but also is not harmful.
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Jan 24, 2023
…ject with proper return codes 4aebd83 db: Change DatabaseCursor::Next to return status enum (Andrew Chow) d79e8dc wallet: Have cursor users use DatabaseCursor directly (Andrew Chow) 7a198bb wallet: Introduce DatabaseCursor RAII class for managing cursor (Andrew Chow) 69efbc0 Move SafeDbt out of BerkeleyBatch (Andrew Chow) Pull request description: Instead of having database cursors be tied to a particular `DatabaseBatch` object and requiring its setup and teardown be separate functions in that batch, we can have cursors be separate RAII classes. This makes it easier to create and destroy cursors as well as having cursors that have slightly different behaviors. Additionally, since reading data from a cursor is a tri-state, this PR changes the return value of the `Next` function (formerly `ReadAtCursor`) to return an Enum rather than the current system of 2 booleans. This greatly simplifies and unifies the code that deals with cursors as now there is no confusion as to what the function returns when there are no records left to be read. Extracted from bitcoin#24914 ACKs for top commit: furszy: diff ACK 4aebd83 theStack: Code-review ACK 4aebd83 Tree-SHA512: 5d0be56a18de5b08c777dd5a73ba5a6ef1e696fdb07d1dca952a88ded07887b7c5c04342f9a76feb2f6fe24a45dc31f094f1f5d9500e6bdf4a44f4edb66dcaa1
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Instead of having database cursors be tied to a particular
DatabaseBatchobject and requiring its setup and teardown be separate functions in that batch, we can have cursors be separate RAII classes. This makes it easier to create and destroy cursors as well as having cursors that have slightly different behaviors.Additionally, since reading data from a cursor is a tri-state, this PR changes the return value of the
Nextfunction (formerlyReadAtCursor) to return an Enum rather than the current system of 2 booleans. This greatly simplifies and unifies the code that deals with cursors as now there is no confusion as to what the function returns when there are no records left to be read.Extracted from #24914