Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
34 changes: 31 additions & 3 deletions frame/support/src/storage/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -502,7 +502,18 @@ pub trait StorageDoubleMap<K1: FullEncode, K2: FullEncode, V: FullCodec> {
KArg1: EncodeLike<K1>,
KArg2: EncodeLike<K2>;

/// Remove all values under the first key.
/// Remove all values under the first key `k1` in the overlay and up to `limit` in the
/// backend.
///
/// All values in the client overlay will be deleted, if there is some `limit` then up to
/// `limit` values are deleted from the client backend, if `limit` is none then all values in
/// the client backend are deleted.
///
/// # Note
///
/// Calling this multiple times per block with a `limit` set leads always to the same keys being
/// removed and the same result being returned. This happens because the keys to delete in the
/// overlay are not taken into account when deleting keys in the backend.
fn remove_prefix<KArg1>(k1: KArg1, limit: Option<u32>) -> sp_io::KillStorageResult
where
KArg1: ?Sized + EncodeLike<K1>;
Expand Down Expand Up @@ -632,7 +643,18 @@ pub trait StorageNMap<K: KeyGenerator, V: FullCodec> {
/// Remove the value under a key.
fn remove<KArg: EncodeLikeTuple<K::KArg> + TupleToEncodedIter>(key: KArg);

/// Remove all values under the partial prefix key.
/// Remove all values starting with `partial_key` in the overlay and up to `limit` in the
/// backend.
///
/// All values in the client overlay will be deleted, if there is some `limit` then up to
/// `limit` values are deleted from the client backend, if `limit` is none then all values in
/// the client backend are deleted.
///
/// # Note
///
/// Calling this multiple times per block with a `limit` set leads always to the same keys being
/// removed and the same result being returned. This happens because the keys to delete in the
/// overlay are not taken into account when deleting keys in the backend.
fn remove_prefix<KP>(partial_key: KP, limit: Option<u32>) -> sp_io::KillStorageResult
where
K: HasKeyPrefix<KP>;
Expand Down Expand Up @@ -1076,11 +1098,17 @@ pub trait StoragePrefixedMap<Value: FullCodec> {
crate::storage::storage_prefix(Self::module_prefix(), Self::storage_prefix())
}

/// Remove all values of the storage in the overlay and up to `limit` in the backend.
/// Remove all values in the overlay and up to `limit` in the backend.
///
/// All values in the client overlay will be deleted, if there is some `limit` then up to
/// `limit` values are deleted from the client backend, if `limit` is none then all values in
/// the client backend are deleted.
///
/// # Note
///
/// Calling this multiple times per block with a `limit` set leads always to the same keys being
/// removed and the same result being returned. This happens because the keys to delete in the
/// overlay are not taken into account when deleting keys in the backend.
fn remove_all(limit: Option<u32>) -> sp_io::KillStorageResult {
sp_io::storage::clear_prefix(&Self::final_prefix(), limit)
}
Expand Down
21 changes: 19 additions & 2 deletions frame/support/src/storage/types/double_map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,18 @@ where
<Self as crate::storage::StorageDoubleMap<Key1, Key2, Value>>::remove(k1, k2)
}

/// Remove all values under the first key.
/// Remove all values under `k1` in the overlay and up to `limit` in the
/// backend.
///
/// All values in the client overlay will be deleted, if there is some `limit` then up to
/// `limit` values are deleted from the client backend, if `limit` is none then all values in
/// the client backend are deleted.
///
/// # Note
///
/// Calling this multiple times per block with a `limit` set leads always to the same keys being
/// removed and the same result being returned. This happens because the keys to delete in the
/// overlay are not taken into account when deleting keys in the backend.
pub fn remove_prefix<KArg1>(k1: KArg1, limit: Option<u32>) -> sp_io::KillStorageResult
where
KArg1: ?Sized + EncodeLike<Key1>,
Expand Down Expand Up @@ -337,11 +348,17 @@ where
>(key1, key2)
}

/// Remove all values of the storage in the overlay and up to `limit` in the backend.
/// Remove all values in the overlay and up to `limit` in the backend.
///
/// All values in the client overlay will be deleted, if there is some `limit` then up to
/// `limit` values are deleted from the client backend, if `limit` is none then all values in
/// the client backend are deleted.
///
/// # Note
///
/// Calling this multiple times per block with a `limit` set leads always to the same keys being
/// removed and the same result being returned. This happens because the keys to delete in the
/// overlay are not taken into account when deleting keys in the backend.
pub fn remove_all(limit: Option<u32>) -> sp_io::KillStorageResult {
<Self as crate::storage::StoragePrefixedMap<Value>>::remove_all(limit)
}
Expand Down
6 changes: 6 additions & 0 deletions frame/support/src/storage/types/map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,12 @@ where
/// All values in the client overlay will be deleted, if there is some `limit` then up to
/// `limit` values are deleted from the client backend, if `limit` is none then all values in
/// the client backend are deleted.
///
/// # Note
///
/// Calling this multiple times per block with a `limit` set leads always to the same keys being
/// removed and the same result being returned. This happens because the keys to delete in the
/// overlay are not taken into account when deleting keys in the backend.
pub fn remove_all(limit: Option<u32>) -> sp_io::KillStorageResult {
<Self as crate::storage::StoragePrefixedMap<Value>>::remove_all(limit)
}
Expand Down
25 changes: 23 additions & 2 deletions frame/support/src/storage/types/nmap.rs
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,18 @@ where
<Self as crate::storage::StorageNMap<Key, Value>>::remove(key)
}

/// Remove all values under the first key.
/// Remove all values starting with `partial_key` in the overlay and up to `limit` in the
/// backend.
///
/// All values in the client overlay will be deleted, if there is some `limit` then up to
/// `limit` values are deleted from the client backend, if `limit` is none then all values in
/// the client backend are deleted.
///
/// # Note
///
/// Calling this multiple times per block with a `limit` set leads always to the same keys being
/// removed and the same result being returned. This happens because the keys to delete in the
/// overlay are not taken into account when deleting keys in the backend.
pub fn remove_prefix<KP>(partial_key: KP, limit: Option<u32>) -> sp_io::KillStorageResult
where
Key: HasKeyPrefix<KP>,
Expand Down Expand Up @@ -277,7 +288,17 @@ where
<Self as crate::storage::StorageNMap<Key, Value>>::migrate_keys::<_>(key, hash_fns)
}

/// Remove all value of the storage.
/// Remove all values in the overlay and up to `limit` in the backend.
///
/// All values in the client overlay will be deleted, if there is some `limit` then up to
/// `limit` values are deleted from the client backend, if `limit` is none then all values in
/// the client backend are deleted.
///
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.

Could add For multiple calls the largest limit is applied.

/// # Note
///
/// Calling this multiple times per block with a `limit` set leads always to the same keys being
/// removed and the same result being returned. This happens because the keys to delete in the
/// overlay are not taken into account when deleting keys in the backend.
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.

Then Note is more about the underlying reason (no persistence of the applied limit in the overlay).

Wonder if renaming limit to limit_backend or backend_fix_limit could be a first hint.

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.

Actually I like your comment on sp-io better.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Actually I like your comment on sp-io better.

Which part exactly? Could you add some suggestions to the pr?

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.

The note that describe the way overlay is handled:

    /// The deletion would always start from `prefix` resulting in the same keys being deleted
	/// every time this function is called with the exact same arguments per block. This happens
	/// because the keys in the overlay are not taken into account when deleting keys in the
	/// backend.

Is more clear to me, but that may be because I know the problematic.
I was thinking a bit about it during lunch and another way to put it would be: limit is only applied over state as it is at the start of the block, ignoring previous calls or something like this.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Talked to @shawntabrizi and he said we can continue with the current docs.

pub fn remove_all(limit: Option<u32>) -> sp_io::KillStorageResult {
<Self as crate::storage::StoragePrefixedMap<Value>>::remove_all(limit)
}
Expand Down
10 changes: 5 additions & 5 deletions primitives/io/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -158,9 +158,7 @@ pub trait Storage {
/// The limit can be used to partially delete a prefix storage in case it is too large
/// to delete in one go (block).
///
/// It returns a boolean false iff some keys are remaining in
/// the prefix after the functions returns. Also returns a `u32` with
/// the number of keys removed from the process.
/// Returns [`KillStorageResult`] to inform about the result.
///
/// # Note
///
Expand All @@ -171,8 +169,10 @@ pub trait Storage {
///
/// Calling this function multiple times per block for the same `prefix` does
/// not make much sense because it is not cumulative when called inside the same block.
/// Use this function to distribute the deletion of a single child trie across multiple
/// blocks.
/// The deletion would always start from `prefix` resulting in the same keys being deleted
/// every time this function is called with the exact same arguments per block. This happens
/// because the keys in the overlay are not taken into account when deleting keys in the
/// backend.
#[version(2)]
fn clear_prefix(&mut self, prefix: &[u8], limit: Option<u32>) -> KillStorageResult {
let (all_removed, num_removed) = Externalities::clear_prefix(*self, prefix, limit);
Expand Down