Block per key-value checksum #11287

cbi42 · 2023-03-10T01:36:39Z

Summary: add option block_protection_bytes_per_key and implementation for block per key-value checksum. The main changes are

checksum construction and verification in block.cc/h
pass the option block_protection_bytes_per_key around (mainly for methods defined in table_cache.h)
unit tests/crash test updates

Tests:

Added unit tests
Crash test: python3 tools/db_crashtest.py blackbox --simple --block_protection_bytes_per_key=1 --write_buffer_size=1048576

Follow up (maybe as a separate PR): make sure corruption status returned from BlockIters are correctly handled.

Performance:
Turning on block per KV protection has a non-trivial negative impact on read performance and costs additional memory.
For memory, each block includes additional 24 bytes for checksum-related states beside checksum itself. For CPU, I set up a DB of size ~1.2GB with 5M keys (32 bytes key and 200 bytes value) which compacts to ~5 SST files (target file size 256 MB) in L6 without compression. I tested readrandom performance with various block cache size (to mimic various cache hit rates):

SETUP
make OPTIMIZE_LEVEL="-O3" USE_LTO=1 DEBUG_LEVEL=0 -j32 db_bench
./db_bench -benchmarks=fillseq,compact0,waitforcompaction,compact,waitforcompaction -write_buffer_size=33554432 -level_compaction_dynamic_level_bytes=true -max_background_jobs=8 -target_file_size_base=268435456 --num=5000000 --key_size=32 --value_size=200 --compression_type=none

BENCHMARK
./db_bench --use_existing_db -benchmarks=readtocache,readrandom[-X10] --num=5000000 --key_size=32 --disable_auto_compactions --reads=1000000 --block_protection_bytes_per_key=[0|1] --cache_size=$CACHESIZE

The readrandom ops/sec looks like the following:
Block cache size:  2GB        1.2GB * 0.9    1.2GB * 0.8     1.2GB * 0.5   8MB
Main              240805     223604         198176           161653       139040
PR prot_bytes=0   238691     226693         200127           161082       141153
PR prot_bytes=1   214983     193199         178532           137013       108211
prot_bytes=1 vs    -10%        -15%          -10.8%          -15%        -23%
prot_bytes=0

The benchmark has a lot of variance, but there was a 5% to 25% regression in this benchmark with different cache hit rates.

facebook-github-bot · 2023-03-10T05:39:49Z

@cbi42 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2023-03-10T21:06:33Z

@cbi42 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2023-03-10T21:09:43Z

@cbi42 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2023-03-10T21:53:25Z

@cbi42 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2023-03-10T22:44:09Z

@cbi42 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2023-03-13T19:35:09Z

@cbi42 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2023-03-13T19:45:03Z

@cbi42 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2023-03-13T21:15:43Z

@cbi42 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2023-03-13T21:17:14Z

@cbi42 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

ajkr

Thanks for the complete test coverage and complete argument passing (that is, no default values for the new parameters)

I have a few files remaining to review

table/block_based/block_test.cc

db_stress_tool/db_stress_common.h

db/kv_checksum.h

ajkr

LGTM! Congrats on finishing this huge feature

table/block_based/block.cc

facebook-github-bot · 2023-04-18T21:45:48Z

@cbi42 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2023-04-19T07:09:55Z

@cbi42 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2023-04-19T19:01:15Z

@cbi42 has updated the pull request. You must reimport the pull request before landing.

cbi42

Thanks for the review! I've update the PR summary with new benchmark results, should be good to review again.

db_stress_tool/db_stress_common.h

db/kv_checksum.h

table/block_based/block_test.cc

table/block_based/block.cc

facebook-github-bot · 2023-04-19T21:39:28Z

@cbi42 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

hx235 · 2023-04-19T22:09:48Z

db/version_edit_handler.cc

@@ -564,12 +564,13 @@ Status VersionEditHandler::LoadTables(ColumnFamilyData* cfd,
  assert(builder_iter->second != nullptr);
  VersionBuilder* builder = builder_iter->second->version_builder();
  assert(builder);
+  const MutableCFOptions* moptions = cfd->GetLatestMutableCFOptions();
  Status s = builder->LoadTableHandlers(


Just so you know : potential merge conflict with #11288

ajkr

LGTM again

ajkr · 2023-04-21T18:03:19Z

table/block_based/block.cc

@@ -1125,6 +1284,7 @@ size_t Block::ApproximateMemoryUsage() const {
  if (read_amp_bitmap_) {
    usage += read_amp_bitmap_->ApproximateMemoryUsage();
  }
+  usage += checksum_size_;


Thanks for remembering this

facebook-github-bot · 2023-04-21T18:50:56Z

@cbi42 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2023-04-22T04:28:17Z

@cbi42 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2023-04-25T16:53:00Z

@cbi42 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2023-04-25T16:53:20Z

@cbi42 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2023-04-25T16:55:26Z

@cbi42 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2023-04-25T19:13:02Z

@cbi42 merged this pull request in 62fc15f.

facebook-github-bot added the CLA Signed label Mar 10, 2023

cbi42 force-pushed the per-kv-checksum-block-cache branch 2 times, most recently from 1cc86ff to 87a559e Compare March 10, 2023 02:27

cbi42 force-pushed the per-kv-checksum-block-cache branch from 48197de to 3802ba1 Compare March 10, 2023 21:09

cbi42 marked this pull request as draft March 13, 2023 19:21

cbi42 force-pushed the per-kv-checksum-block-cache branch from 0a0803f to d5c68f5 Compare March 13, 2023 21:15

cbi42 requested a review from ajkr March 13, 2023 21:16

cbi42 marked this pull request as ready for review March 13, 2023 21:16

ajkr reviewed Apr 11, 2023

View reviewed changes

table/block_based/block_test.cc Outdated Show resolved Hide resolved

table/block_based/block_test.cc Outdated Show resolved Hide resolved

db_stress_tool/db_stress_common.h Show resolved Hide resolved

db/kv_checksum.h Outdated Show resolved Hide resolved

ajkr approved these changes Apr 11, 2023

View reviewed changes

table/block_based/block.cc Outdated Show resolved Hide resolved

table/block_based/block.cc Outdated Show resolved Hide resolved

cbi42 force-pushed the per-kv-checksum-block-cache branch from d5c68f5 to 9d8e70a Compare April 18, 2023 21:45

cbi42 commented Apr 19, 2023

View reviewed changes

hx235 reviewed Apr 19, 2023

View reviewed changes

ajkr approved these changes Apr 21, 2023

View reviewed changes

cbi42 force-pushed the per-kv-checksum-block-cache branch from 6c0407d to eb622b1 Compare April 21, 2023 18:50

cbi42 force-pushed the per-kv-checksum-block-cache branch from eb622b1 to 7ce6820 Compare April 25, 2023 16:52

cbi42 added 19 commits April 25, 2023 09:53

block per kv checksum

a66dae0

fix fuzzer

80eb3b5

Fix wrong parameter

8e42a94

Fix CI failure and add error handling in Initialize*BlockProtectionInfo

d0833cf

remove extra ;

fa10142

prevent overflow

2f0d012

error handling in SeekForPrevImpl and increase test coverage

c6013aa

deflake corruption tests and better naming for syncpoints

954f041

Fix for failed CI test

51abd5b

outdated code/comment

d9b7f2a

randomize block_protection in crash test

59fb802

move Encode/Decode functions into ProtectionInfo class

96da20a

Remove unrelated test and simpliy initialization test

f7d5003

Remove checksum verification in a restart interval.

ee5ecd1

Remove Block::VerifyChecksum.

7d300d6

Include checksum in approximate memory usage and updated some comments.

84cfc07

Fix a flaky test.

109a6de

Fix build-window build failure

fd09a2e

HISTORY.md

240b522

cbi42 force-pushed the per-kv-checksum-block-cache branch from 7ce6820 to 240b522 Compare April 25, 2023 16:53

facebook-github-bot closed this in 62fc15f Apr 25, 2023

facebook-github-bot added the Merged label Apr 25, 2023

igorcanadi mentioned this pull request Jan 17, 2024

[SYS-6913] Upgrade RocksDB-Cloud to 8.9.1 rockset/rocksdb-cloud#315

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Block per key-value checksum #11287

Block per key-value checksum #11287

cbi42 commented Mar 10, 2023 •

edited

facebook-github-bot commented Mar 10, 2023

facebook-github-bot commented Mar 10, 2023

facebook-github-bot commented Mar 10, 2023

facebook-github-bot commented Mar 10, 2023

facebook-github-bot commented Mar 10, 2023

facebook-github-bot commented Mar 13, 2023

facebook-github-bot commented Mar 13, 2023

facebook-github-bot commented Mar 13, 2023

facebook-github-bot commented Mar 13, 2023

ajkr left a comment

ajkr left a comment

facebook-github-bot commented Apr 18, 2023

facebook-github-bot commented Apr 19, 2023

facebook-github-bot commented Apr 19, 2023

cbi42 left a comment

facebook-github-bot commented Apr 19, 2023

hx235 Apr 19, 2023

ajkr left a comment

ajkr Apr 21, 2023

facebook-github-bot commented Apr 21, 2023

facebook-github-bot commented Apr 22, 2023

facebook-github-bot commented Apr 25, 2023

facebook-github-bot commented Apr 25, 2023

facebook-github-bot commented Apr 25, 2023

facebook-github-bot commented Apr 25, 2023

Block per key-value checksum #11287

Block per key-value checksum #11287

Conversation

cbi42 commented Mar 10, 2023 • edited

facebook-github-bot commented Mar 10, 2023

facebook-github-bot commented Mar 10, 2023

facebook-github-bot commented Mar 10, 2023

facebook-github-bot commented Mar 10, 2023

facebook-github-bot commented Mar 10, 2023

facebook-github-bot commented Mar 13, 2023

facebook-github-bot commented Mar 13, 2023

facebook-github-bot commented Mar 13, 2023

facebook-github-bot commented Mar 13, 2023

ajkr left a comment

Choose a reason for hiding this comment

ajkr left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Apr 18, 2023

facebook-github-bot commented Apr 19, 2023

facebook-github-bot commented Apr 19, 2023

cbi42 left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Apr 19, 2023

hx235 Apr 19, 2023

Choose a reason for hiding this comment

ajkr left a comment

Choose a reason for hiding this comment

ajkr Apr 21, 2023

Choose a reason for hiding this comment

facebook-github-bot commented Apr 21, 2023

facebook-github-bot commented Apr 22, 2023

facebook-github-bot commented Apr 25, 2023

facebook-github-bot commented Apr 25, 2023

facebook-github-bot commented Apr 25, 2023

facebook-github-bot commented Apr 25, 2023

cbi42 commented Mar 10, 2023 •

edited