-
Notifications
You must be signed in to change notification settings - Fork 466
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
ledger: convert FC unmarshalled nil value to empty byte slice on DB write KVs #5225
Conversation
8f0a2a7
to
f871008
Compare
f871008
to
6539d28
Compare
Codecov Report
@@ Coverage Diff @@
## master #5225 +/- ##
==========================================
- Coverage 53.73% 53.71% -0.03%
==========================================
Files 444 444
Lines 55671 55680 +9
==========================================
- Hits 29913 29906 -7
- Misses 23426 23440 +14
- Partials 2332 2334 +2
... and 6 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
031ffba
to
9ebb1d0
Compare
4ef16a7
to
832e43c
Compare
832e43c
to
b5d3b51
Compare
Do we want to do the migration? I guess it is probably cheap to do because there aren't a lot of KVs out in the wild yet? |
I am not really sure, for I am unaware of how many nil values out there. But the migration should be cheap, for I guess the nil values in the table caused by FC should be sparse. (just guessing) |
73b314a
to
3739d3f
Compare
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.
Looks good, thanks.
Summary
This is an improvement for DB on write, though it does not have effect on current sqlite3, it might matter if we take new DB backend (some Key-value based DB).
Since
encoded.KVRecordV6
iscodec:,omitempty,omitemptyarray
, when we have an instance ofencoded.KVRecordV6
, WLOG we talk about empty box, then an empty byte slice[]byte{}
would be unmarshalled tonil
, and written to DB.The current status is, writing to DB with a
nil
value tied to a key doesn't really matter, for sqlite3 takesnil
and[]byte{}
as same empty BLOB. But for the sake of consistency, we should convert the value to[]byte{}
.Also, when we recheck with the logic of a round-by-round catchup, a box deletion would incur
kvmod.val = nil
, andaccountsNewRoundImpl
inacctdelta.go
would treat this as a KV pair deletion in DB.Thus, I think it would be good to convert a
nil
value unmarshal result to an empty byte slice[]byte{}
.Test Plan
Consider the following test:
kvstore