-
Notifications
You must be signed in to change notification settings - Fork 1k
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
stagedSync: Optimize prune old chunks #10019
Conversation
eth/stagedsync/stage_log_index.go
Outdated
if err := pruneLogKeyCollector.Collect(k, nil); err != nil { | ||
return err | ||
} | ||
c.DeleteCurrent() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- err not checked
- plz use
tx.Delete(table, k)
. Because seems some version of mdbx has bug when same cursor used forfor
iteration andDeleteCurrent
- we did workaround it by using 2 cursors ortx.Delete
, tnx. I'm not sure which versions are affected - because not easy to reproduce.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tx.DeleteCurrent
pattern is used at many places in the code base. Are you also saying all of them need fixing too, then? Because the way i see it,DeleteCurrent
can only be used in a for loop like that.- What is the performance impact of using
tx.Delete
here, compared toDeleteCurrent
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DeleteCurrent is not bad - use 1 cursor to iterate and in same time DeleteCurrent - bad in some mdbx versions.
No performance impact (of course it’s there - O(1) vs O(n log n), but mdbx’s methods itself are fast-enough, bottleneck is usually in “touching cold data”, but you touching it by loop anyway).
Don’t need modify other places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, for now, using tx.Delete
in its place
Summary
Fixes prune point for log (+index)
kv.Log
entries, delete in the initial looppruneTo
block number in thePruneState
- this will begin pruning from that point. Earlier thepruneFrom
point being passed in was buggy as it used some other assumption for this value