Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

Fix the boundary inconsistency between delete_file_in_range and delete_range #27201

Merged
yhchiang-sol merged 1 commit into from
Aug 31, 2022
Merged

Fix the boundary inconsistency between delete_file_in_range and delete_range #27201

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions ledger/src/blockstore.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2059,8 +2059,8 @@ impl Blockstore {
)
}

/// Toggles the active primary index between `0` and `1`, and clears the stored max-slot of the
/// frozen index in preparation for pruning.
/// Toggles the active primary index between `0` and `1`, and clears the
/// stored max-slot of the frozen index in preparation for pruning.
fn toggle_transaction_status_index(
&self,
batch: &mut WriteBatch,
Expand Down
42 changes: 19 additions & 23 deletions ledger/src/blockstore/blockstore_purge.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,8 +145,8 @@ impl Blockstore {
self.run_purge_with_stats(from_slot, to_slot, purge_type, &mut PurgeStats::default())
}

/// A helper function to `purge_slots` that executes the ledger clean up
/// from `from_slot` to `to_slot`.
/// A helper function to `purge_slots` that executes the ledger clean up.
/// The cleanup applies to \[`from_slot`, `to_slot`\].
///
/// When `from_slot` is 0, any sst-file with a key-range completely older
/// than `to_slot` will also be deleted.
Expand All @@ -161,9 +161,6 @@ impl Blockstore {
.db
.batch()
.expect("Database Error: Failed to get write batch");
// delete range cf is not inclusive
Comment thread
yhchiang-sol marked this conversation as resolved.
let to_slot = to_slot.saturating_add(1);

let mut delete_range_timer = Measure::start("delete_range");
let mut columns_purged = self
.db
Expand Down Expand Up @@ -444,15 +441,18 @@ impl Blockstore {
/// deserializing each slot being purged and iterating through all
/// transactions to determine the keys of individual records.
///
/// The purge range applies to \[`from_slot`, `to_slot`\].
///
/// **This method is very slow.**
fn purge_special_columns_exact(
&self,
batch: &mut WriteBatch,
from_slot: Slot,
to_slot: Slot, // Exclusive
to_slot: Slot,
) -> Result<()> {
let mut index0 = self.transaction_status_index_cf.get(0)?.unwrap_or_default();
let mut index1 = self.transaction_status_index_cf.get(1)?.unwrap_or_default();
let to_slot = to_slot.saturating_add(1);
Copy link
Copy Markdown
Contributor

@ryoqun ryoqun Sep 1, 2022

Choose a reason for hiding this comment

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

nit: there's more rusty way:

for slot in from_slot..=to_slot {
    ...
}

Copy link
Copy Markdown
Contributor Author

@yhchiang-sol yhchiang-sol Sep 1, 2022

Choose a reason for hiding this comment

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

Thanks for spotting this. Will have a quick fix for this.

for slot in from_slot..to_slot {
let slot_entries = self.get_any_valid_slot_entries(slot, 0);
let transactions = slot_entries
Expand Down Expand Up @@ -501,22 +501,18 @@ impl Blockstore {
if let Some(purged_index) = self.toggle_transaction_status_index(
write_batch,
w_active_transaction_status_index,
to_slot,
to_slot + 1,
Copy link
Copy Markdown
Contributor

@ryoqun ryoqun Sep 1, 2022

Choose a reason for hiding this comment

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

like this

Copy link
Copy Markdown
Contributor Author

@yhchiang-sol yhchiang-sol Sep 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'm in the process of revisiting deletion- and ledger-cleanup-related code and adding missing tests if any. Just want to leave a comment here that this has been covered by the existing check.

Below is the test failure log if I remove +1 in the above statement.

---- blockstore::blockstore_purge::tests::test_purge_transaction_status stdout ----
thread 'blockstore::blockstore_purge::tests::test_purge_transaction_status' panicked at 'assertion failed: `(left == right)`
  left: `0`,
 right: `2`', ledger/src/blockstore/blockstore_purge.rs:752:9
stack backtrace:
   0: rust_begin_unwind
             at /rustc/4b91a6ea7258a947e59c6522cd5898e7c0a6a88f/library/std/src/panicking.rs:584:5
   1: core::panicking::panic_fmt
             at /rustc/4b91a6ea7258a947e59c6522cd5898e7c0a6a88f/library/core/src/panicking.rs:142:14
   2: core::panicking::assert_failed_inner
   3: core::panicking::assert_failed
             at /rustc/4b91a6ea7258a947e59c6522cd5898e7c0a6a88f/library/core/src/panicking.rs:181:5
   4: solana_ledger::blockstore::blockstore_purge::tests::test_purge_transaction_status
             at ./src/blockstore/blockstore_purge.rs:752:9
   5: solana_ledger::blockstore::blockstore_purge::tests::test_purge_transaction_status::{{closure}}
             at ./src/blockstore/blockstore_purge.rs:615:5
   6: core::ops::function::FnOnce::call_once
             at /rustc/4b91a6ea7258a947e59c6522cd5898e7c0a6a88f/library/core/src/ops/function.rs:248:5
   7: core::ops::function::FnOnce::call_once
             at /rustc/4b91a6ea7258a947e59c6522cd5898e7c0a6a88f/library/core/src/ops/function.rs:248:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

)? {
*columns_purged &= self
.db
.delete_range_cf::<cf::TransactionStatus>(
write_batch,
purged_index,
purged_index + 1,
)
.delete_range_cf::<cf::TransactionStatus>(write_batch, purged_index, purged_index)
.is_ok()
& self
.db
.delete_range_cf::<cf::AddressSignatures>(
write_batch,
purged_index,
purged_index + 1,
purged_index,
)
.is_ok();
}
Expand Down Expand Up @@ -783,7 +779,7 @@ pub mod tests {
);
}

fn clear_and_repopulate_transaction_statuses(
fn clear_and_repopulate_transaction_statuses_for_test(
blockstore: &mut Blockstore,
index0_max_slot: u64,
index1_max_slot: u64,
Expand All @@ -795,11 +791,11 @@ pub mod tests {
.unwrap();
blockstore
.db
.delete_range_cf::<cf::TransactionStatus>(&mut write_batch, 0, 3)
.delete_range_cf::<cf::TransactionStatus>(&mut write_batch, 0, 2)
.unwrap();
blockstore
.db
.delete_range_cf::<cf::TransactionStatusIndex>(&mut write_batch, 0, 3)
.delete_range_cf::<cf::TransactionStatusIndex>(&mut write_batch, 0, 2)
.unwrap();
blockstore.db.write(write_batch).unwrap();
blockstore.initialize_transaction_status_index().unwrap();
Expand Down Expand Up @@ -897,7 +893,7 @@ pub mod tests {
let index1_max_slot = 19;

// Test purge outside bounds
clear_and_repopulate_transaction_statuses(
clear_and_repopulate_transaction_statuses_for_test(
&mut blockstore,
index0_max_slot,
index1_max_slot,
Expand Down Expand Up @@ -944,7 +940,7 @@ pub mod tests {
drop(status_entry_iterator);

// Test purge inside index 0
clear_and_repopulate_transaction_statuses(
clear_and_repopulate_transaction_statuses_for_test(
&mut blockstore,
index0_max_slot,
index1_max_slot,
Expand Down Expand Up @@ -993,7 +989,7 @@ pub mod tests {
drop(status_entry_iterator);

// Test purge inside index 0 at upper boundary
clear_and_repopulate_transaction_statuses(
clear_and_repopulate_transaction_statuses_for_test(
&mut blockstore,
index0_max_slot,
index1_max_slot,
Expand Down Expand Up @@ -1044,7 +1040,7 @@ pub mod tests {
drop(status_entry_iterator);

// Test purge inside index 1 at lower boundary
clear_and_repopulate_transaction_statuses(
clear_and_repopulate_transaction_statuses_for_test(
&mut blockstore,
index0_max_slot,
index1_max_slot,
Expand Down Expand Up @@ -1092,7 +1088,7 @@ pub mod tests {
drop(status_entry_iterator);

// Test purge across index boundaries
clear_and_repopulate_transaction_statuses(
clear_and_repopulate_transaction_statuses_for_test(
&mut blockstore,
index0_max_slot,
index1_max_slot,
Expand Down Expand Up @@ -1142,7 +1138,7 @@ pub mod tests {
drop(status_entry_iterator);

// Test purge include complete index 1
clear_and_repopulate_transaction_statuses(
clear_and_repopulate_transaction_statuses_for_test(
&mut blockstore,
index0_max_slot,
index1_max_slot,
Expand Down Expand Up @@ -1189,7 +1185,7 @@ pub mod tests {
drop(status_entry_iterator);

// Test purge all
clear_and_repopulate_transaction_statuses(
clear_and_repopulate_transaction_statuses_for_test(
&mut blockstore,
index0_max_slot,
index1_max_slot,
Expand Down
21 changes: 17 additions & 4 deletions ledger/src/blockstore_db.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -455,6 +455,7 @@ impl Rocks {
Ok(())
}

/// Delete files whose slot range is within \[`from`, `to`\].
fn delete_file_in_range_cf(
&self,
cf: &ColumnFamily,
Expand Down Expand Up @@ -1119,17 +1120,23 @@ impl Database {
Ok(fs_extra::dir::get_size(&self.path)?)
}

// Adds a range to delete to the given write batch
/// Adds a \[`from`, `to`\] range to delete to the given write batch
pub fn delete_range_cf<C>(&self, batch: &mut WriteBatch, from: Slot, to: Slot) -> Result<()>
where
C: Column + ColumnName,
{
let cf = self.cf_handle::<C>();
// Note that the default behavior of rocksdb's delete_range_cf deletes
// files within [from, to), while our purge logic applies to [from, to].
//
// For consistency, we make our delete_range_cf works for [from, to] by
// adjusting the `to` slot range by 1.
let from_index = C::as_index(from);
let to_index = C::as_index(to);
let to_index = C::as_index(to.saturating_add(1));
batch.delete_range_cf::<C>(cf, from_index, to_index)
}

/// Delete files whose slot range is within \[`from`, `to`\].
pub fn delete_file_in_range_cf<C>(&self, from: Slot, to: Slot) -> Result<()>
where
C: Column + ColumnName,
Expand Down Expand Up @@ -1439,11 +1446,17 @@ impl<'a> WriteBatch<'a> {
self.map[C::NAME]
}

pub fn delete_range_cf<C: Column>(
/// Adds a \[`from`, `to`) range deletion entry to the batch.
///
/// Note that the \[`from`, `to`) deletion range of WriteBatch::delete_range_cf
/// is different from \[`from`, `to`\] of Database::delete_range_cf as we makes
/// the semantics of Database::delete_range_cf matches the blockstore purge
/// logic.
fn delete_range_cf<C: Column>(
Copy link
Copy Markdown
Contributor

@ryoqun ryoqun Sep 1, 2022

Choose a reason for hiding this comment

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

@yhchiang-sol I'm very happy about our new consistent interval handling with [from, to]. :)

that said, I think we should put the range manipulation code as deep as possible for encapsulation.

so, i think this non-pub fn might be a good place to actually .saturating_add(1). That's because it looks like WriteBatch::delete_range_cf is only called by Database::delete_range_cf?

Then, we can remove these rather extra justification comment about different semantics put at the docstring in WriteBatch::delete_range_cf

is there strong reason we're adjusting the to at Database::delete_range_cf specifically?

Copy link
Copy Markdown
Contributor Author

@yhchiang-sol yhchiang-sol Sep 1, 2022

Choose a reason for hiding this comment

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

The type here is C::Index, which could be u64, (u64, u64), Pubkey, Signature, (u64, Signature, Slot), and (u64, Pubkey, Slot, Signature). We will need to implement the C::saturating_add for each of them. Might be worth-trying I think, although some types might be tricky.

If we want to move lower to the rocksdb delete_range_cf, then is more difficult to perform +1 as it takes arbitrary byte array.

A cleaner solution is to make RocksDB's range delete optionally perform the inclusive deletion based on the WriteOptions where we will add a new boolean indicating inclusive deletion, but I guess this would take a much longer route as we need to carry this information into range-deletion key format and update the internal range-deletion logic to honor this.

So probably good for now I think.

&mut self,
cf: &ColumnFamily,
from: C::Index,
to: C::Index,
to: C::Index, // exclusive
) -> Result<()> {
self.write_batch
.delete_range_cf(cf, C::key(from), C::key(to));
Expand Down