Skip to content

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

Merged
clemahieu merged 4 commits intonanocurrency:developfrom
rsnano-node:clear-unchecked-table
May 15, 2023
Merged

Add LMDB and RocksDB upgrades to v22: Remove unchecked table#4131
clemahieu merged 4 commits intonanocurrency:developfrom
rsnano-node:clear-unchecked-table

Conversation

@simpago
Copy link
Copy Markdown
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
Copy Markdown
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
@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 force-pushed the clear-unchecked-table branch from b9fae83 to c7d77ed Compare May 4, 2023 13:08
@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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in 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