Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Parity-db Change missing implementation. #11049

Merged
arkpar merged 11 commits into from
May 6, 2022
Merged

Parity-db Change missing implementation. #11049

arkpar merged 11 commits into from
May 6, 2022

Conversation

@cheme
Copy link
Contributor

@cheme cheme commented Mar 16, 2022 •
edited
Loading

This pr is a minimal implementation of Change::Store, Change::Reference, Change::Release, in order for parity db to support storage chain (those operator are only in this case).

The implementation is really not optimal: when using reference, we should not fetch the value, a parity-db specific instruction should be use (only need to update the internal storage rc), with current commit api adding a new instruction requires a bit of rework so having this quick PR first make sense to me.

In case column do not use rc, I choose to not use the default implementation from kvdb as it indicates that something should be change.

Polkadot companion: paritytech/polkadot#5472

@cheme cheme added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit. labels Mar 16, 2022
arkpar
arkpar reviewed Mar 16, 2022
View reviewed changes
client/db/src/parity_db.rs Outdated
Change::Reference(col, key) =>
if ref_counted_column(col) {
// FIXME accessing value is not strictly needed, optimize this in parity-db.
let value = <Self as Database<H>>::get(self, col, key.as_ref());
Copy link
Member

@arkpar arkpar Mar 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

commit in parity_db could use a more clear change API indeed. I think multiple references/releases of the same key in a single commit are not supported at the moment. It should work fine actually.

Copy link
Contributor Author

@cheme cheme Mar 16, 2022

Choose a reason for hiding this comment

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

yes, I also realize this morning that we always read the same counter value.
I did not fix it, as for storage chain it is not an issue and I do not know of other usage of these Change variants.

arkpar
arkpar reviewed Mar 16, 2022
View reviewed changes
arkpar
arkpar reviewed Mar 16, 2022
View reviewed changes
arkpar
arkpar reviewed Mar 16, 2022
View reviewed changes
cheme and others added 3 commits March 16, 2022 18:25
@cheme @arkpar
Update client/db/src/parity_db.rs
55f76c8 
Co-authored-by: Arkadiy Paronyan <[email protected]>
@cheme @arkpar
Update client/db/src/parity_db.rs
96098fc 
Co-authored-by: Arkadiy Paronyan <[email protected]>
@cheme @arkpar
Update client/db/src/parity_db.rs
755ad97 
Co-authored-by: Arkadiy Paronyan <[email protected]>
@stale
Copy link

stale bot commented Apr 15, 2022

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Apr 15, 2022
@cheme
Copy link
Contributor Author

cheme commented Apr 15, 2022

I totally lost track of this PR, still needed.

@cheme cheme removed the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Apr 15, 2022
@cheme cheme requested a review from bkchr April 15, 2022 18:00
@cheme cheme requested a review from 0x7CFE May 5, 2022 16:23
bkchr
bkchr approved these changes May 5, 2022
View reviewed changes
client/db/src/parity_db.rs Outdated
if ref_counted_column(col) {
(col as u8, key.as_ref().to_vec(), Some(value))
} else {
panic!("Change::Store is only used for ref counted columns")
Copy link
Member

@bkchr bkchr May 5, 2022

Choose a reason for hiding this comment

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

If that is the case, why do we still need the if around it?

client/db/src/parity_db.rs Outdated
if ref_counted_column(col) {
(col as u8, key.as_ref().to_vec(), Some(value))
} else {
panic!("Change::Store is only used for ref counted columns")
Copy link
Member

@bkchr bkchr May 5, 2022

Choose a reason for hiding this comment

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

Suggested change
panic!("Change::Store is only used for ref counted columns")
unreachable!("Change::Store is only used for ref counted columns")

client/db/src/parity_db.rs Outdated
if ref_counted_column(col) {
(col as u8, key.as_ref().to_vec(), None)
} else {
panic!("Change::Release is only used for ref counted columns")
Copy link
Member

@bkchr bkchr May 5, 2022

Choose a reason for hiding this comment

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

Suggested change
panic!("Change::Release is only used for ref counted columns")
unreachable!("Change::Release is only used for ref counted columns")

client/db/src/parity_db.rs Outdated
let value = <Self as Database<H>>::get(self, col, key.as_ref());
(col as u8, key.as_ref().to_vec(), value)
} else {
panic!("Change::Reference is only used for ref counted columns")
Copy link
Member

@bkchr bkchr May 5, 2022

Choose a reason for hiding this comment

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

Suggested change
panic!("Change::Reference is only used for ref counted columns")
unreachable!("Change::Reference is only used for ref counted columns")

@cheme
Copy link
Contributor Author

cheme commented May 6, 2022

@bkchr , @arkpar , I did change the code a bit by returning error with the commit method of the adapter (previously error on commit was panicking).
May not be a great idea, but I think it can be a good direction. I can revert though (ensuring no code regression).

@arkpar
Copy link
Member

arkpar commented May 6, 2022

bot merge

@paritytech-processbot
Copy link

paritytech-processbot bot commented May 6, 2022

Waiting for commit status.

@paritytech-processbot
Copy link

paritytech-processbot bot commented May 6, 2022

Merge cancelled due to error. Error: Statuses failed for 63f2770

@cheme cheme mentioned this pull request May 6, 2022
Merged
@cheme
Copy link
Contributor Author

cheme commented May 6, 2022

bot merge

@paritytech-processbot
Copy link

paritytech-processbot bot commented May 6, 2022

Error: Statuses failed for 63f2770

@arkpar arkpar merged commit f7df8fb into paritytech:master May 6, 2022
godcodehunter pushed a commit to sensoriumxr/substrate that referenced this pull request Jun 22, 2022
@cheme @arkpar @godcodehunter
Parity-db Change missing implementation. (paritytech#11049)
07c6b06 
* support for release as in kvdb (only if no rc).

* Start impl

* minimal implementation for paritydb rc

* Update client/db/src/parity_db.rs

Co-authored-by: Arkadiy Paronyan <[email protected]>

* Update client/db/src/parity_db.rs

Co-authored-by: Arkadiy Paronyan <[email protected]>

* Update client/db/src/parity_db.rs

Co-authored-by: Arkadiy Paronyan <[email protected]>

* Commit not panicking in DbAdapter

* errors from string

* update parity db version

Co-authored-by: Arkadiy Paronyan <[email protected]>
DaviRain-Su pushed a commit to octopus-network/substrate that referenced this pull request Aug 23, 2022
@cheme @arkpar @DaviRain-Su
Parity-db Change missing implementation. (paritytech#11049)
2215157 
* support for release as in kvdb (only if no rc).

* Start impl

* minimal implementation for paritydb rc

* Update client/db/src/parity_db.rs

Co-authored-by: Arkadiy Paronyan <[email protected]>

* Update client/db/src/parity_db.rs

Co-authored-by: Arkadiy Paronyan <[email protected]>

* Update client/db/src/parity_db.rs

Co-authored-by: Arkadiy Paronyan <[email protected]>

* Commit not panicking in DbAdapter

* errors from string

* update parity db version

Co-authored-by: Arkadiy Paronyan <[email protected]>
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
@cheme @arkpar @ark0f
Parity-db Change missing implementation. (paritytech#11049)
dff0626 
* support for release as in kvdb (only if no rc).

* Start impl

* minimal implementation for paritydb rc

* Update client/db/src/parity_db.rs

Co-authored-by: Arkadiy Paronyan <[email protected]>

* Update client/db/src/parity_db.rs

Co-authored-by: Arkadiy Paronyan <[email protected]>

* Update client/db/src/parity_db.rs

Co-authored-by: Arkadiy Paronyan <[email protected]>

* Commit not panicking in DbAdapter

* errors from string

* update parity db version

Co-authored-by: Arkadiy Paronyan <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@0x7CFE 0x7CFE Awaiting requested review from 0x7CFE

2 more reviewers

@arkpar arkpar arkpar approved these changes

@bkchr bkchr bkchr approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@cheme @arkpar @bkchr