Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add LMDB and RocksDB upgrades to v22: Remove unchecked table #4131

Merged
merged 4 commits into from
May 15, 2023

Conversation

simpago
Copy link
Contributor

@simpago simpago commented Feb 15, 2023

In pull request #4021 the binary representation of unchecked_info was changed. We have to clear the unchecked table, because the new binary format is not compatible with the old one.

This commit only clears the unchecked table for the LMDB implementation. The RocksDB implementation doesn't have an upgrade mechanism yet.

@thsfs thsfs added the database structure If the database changes it needs updating in the nanodb repository label Apr 17, 2023
Copy link
Contributor

@thsfs thsfs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For binary compatibility the respective unchecked table handle should be removed as well. This will require some clean up work in the the unchecked_map.?pp and secure/store.?pp files, because they are currently relying on this table to exist even not using it.

Suggest sending another PR for the clean up work before resuming with this.

@thsfs thsfs force-pushed the clear-unchecked-table branch 2 times, most recently from ecd1f03 to 865c79f Compare April 25, 2023 20:47
@thsfs thsfs requested review from clemahieu and thsfs and removed request for thsfs April 25, 2023 20:48
nano/node/lmdb/block_store.hpp Outdated Show resolved Hide resolved
nano/node/lmdb/lmdb.cpp Outdated Show resolved Hide resolved
nano/node/lmdb/lmdb.cpp Outdated Show resolved Hide resolved
@thsfs thsfs requested a review from clemahieu April 25, 2023 21:42
In pull request nanocurrency#4021 the binary representation of `unchecked_info` was
changed. We have to clear the unchecked table, because the new binary
format is not compatible with the old one.

This commit only clears the unchecked table for the LMDB implementation.
The RocksDB implementation doesn't have an upgrade mechanism yet.

- Delete the unchecked DB from the LMDB environment
- Simplify the unchecked handle
@thsfs thsfs changed the title Add LMDB upgrade to v22: Clear unchecked table Add LMDB and RocksDB upgrades to v22: Remove unchecked table May 5, 2023
generate_tombstone_map ();
small_table_factory.reset (::rocksdb::NewBlockBasedTableFactory (get_small_table_options ()));

// TODO: get_db_options () registers a listener for resetting tombstones, needs to check if it is a problem calling it more than once.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good thing to be reviewed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Encapsulating this to only be called once looks correct.

@thsfs thsfs self-requested a review May 5, 2023 20:52
@thsfs thsfs added this to the V25.0 milestone May 5, 2023
@thsfs thsfs added the documentation This item indicates the need for or supplies updated or expanded documentation label May 5, 2023
Copy link
Contributor

@clemahieu clemahieu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was able to verify the table was dropped and version number increased.

@clemahieu clemahieu merged commit 0f19fab into nanocurrency:develop May 15, 2023
@qwahzi qwahzi mentioned this pull request May 25, 2023
7 tasks
@simpago simpago deleted the clear-unchecked-table branch August 11, 2023 12:22
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
database structure If the database changes it needs updating in the nanodb repository documentation This item indicates the need for or supplies updated or expanded documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants