Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

chore: use readonly providers in StorageWriter #9822

Merged
merged 2 commits into from
Jul 26, 2024

Conversation

Rjected
Copy link
Member

@Rjected Rjected commented Jul 25, 2024

This changes StorageWriter to be generic over the transaction, allowing methods which only require a read transaction to use a read-only database provider.

@Rjected Rjected added C-enhancement New feature or request A-db Related to the database labels Jul 25, 2024
Copy link
Member

@emhane emhane left a comment

Choose a reason for hiding this comment

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

nice helpful docs (from prev pr, but still)

Comment on lines +122 to +125
impl<'a, 'b, TX> StorageWriter<'a, 'b, TX>
where
TX: DbTx,
{
Copy link
Member

Choose a reason for hiding this comment

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

I like how you implement these methods only for ro tx, that enforces future development to uphold this performant pattern in terms of not locking db for write when not necessary

Comment on lines +350 to +353
impl<'a, 'b, TX> StateWriter for StorageWriter<'a, 'b, TX>
where
TX: DbTxMut + DbTx,
{
Copy link
Member

Choose a reason for hiding this comment

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

unnecessary to combine writing receipts to static files and to db in one transaction, defeats the purpose a bit of having the separation between rw and ro tx in the first place for StorageWriter. would be better if this was separated into two functions called sequentially wherever write_to_stroage is called now. a little minus for dev ex but big plus for perf.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know if it's possible to split it up that way, or what the right API would be, because whether or not static files are written to depends on the prune config (ie in append_receipts_from_blocks). Open to ideas though

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

lgtm, being able to provide the txkind makes a lot of sense

@Rjected Rjected added this pull request to the merge queue Jul 26, 2024
Merged via the queue into main with commit f734e61 Jul 26, 2024
33 checks passed
@Rjected Rjected deleted the dan/use-read-only-provider-static-file-writes branch July 26, 2024 00:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-db Related to the database C-enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants