Fix kBlockCacheTier read when merge-chain base value is in a blob file by ajkr · Pull Request #12462 · facebook/rocksdb

ajkr · 2024-03-20T21:03:39Z

The original goal is to propagate failures from GetContext::SaveValue() -> GetContext::GetBlobValue() -> BlobFetcher::FetchBlob() up to the user. This call sequence happens when a merge chain ends with a base value in a blob file.

There's also fixes for bugs encountered along the way where non-ok statuses were ignored/overwritten, and a bit of plumbing work for functions that had no capability to return a status.

Test Plan:

A repro command

db=/dev/shm/dbstress_db ; exp=/dev/shm/dbstress_exp ; rm -rf $db $exp ; mkdir -p $db $exp
./db_stress \
        --clear_column_family_one_in=0 \
        --test_batches_snapshots=0 \
        --write_fault_one_in=0 \
        --use_put_entity_one_in=0 \
        --prefixpercent=0 \
        --read_fault_one_in=0 \
        --readpercent=0 \
        --reopen=0 \
        --set_options_one_in=10000 \
        --delpercent=0 \
        --delrangepercent=0 \
        --open_metadata_write_fault_one_in=0 \
        --open_read_fault_one_in=0 \
        --open_write_fault_one_in=0 \
        --destroy_db_initially=0 \
        --ingest_external_file_one_in=0 \
        --iterpercent=0 \
        --nooverwritepercent=0 \
        --db=$db \
        --enable_blob_files=1 \
        --expected_values_dir=$exp \
        --max_background_compactions=20 \
        --max_bytes_for_level_base=2097152 \
        --max_key=100000 \
        --min_blob_size=0 \
        --open_files=-1 \
        --ops_per_thread=100000000 \
        --prefix_size=-1 \
        --target_file_size_base=524288 \
        --use_merge=1 \
        --value_size_mult=32 \
        --write_buffer_size=524288 \
        --writepercent=100

It used to fail like:

...
frame #9: 0x00007fc63903bc93 libc.so.6`__GI___assert_fail(assertion="HasDefaultColumn(columns)", file="fbcode/internal_repo_rocksdb/repo/db/wide/wide_columns_helper.h", line=33, function="static const rocksdb::Slice &rocksdb::WideColumnsHelper::GetDefaultColumn(const rocksdb::WideColumns &)") at assert.c:101:3
frame #10: 0x00000000006f7e92 db_stress`rocksdb::Version::Get(rocksdb::ReadOptions const&, rocksdb::LookupKey const&, rocksdb::PinnableSlice*, rocksdb::PinnableWideColumns*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>*, rocksdb::Status*, rocksdb::MergeContext*, unsigned long*, rocksdb::PinnedIteratorsManager*, bool*, bool*, unsigned long*, rocksdb::ReadCallback*, bool*, bool) [inlined] rocksdb::WideColumnsHelper::GetDefaultColumn(columns=size=0) at wide_columns_helper.h:33
frame #11: 0x00000000006f7e76 db_stress`rocksdb::Version::Get(this=0x00007fc5ec763000, read_options=<unavailable>, k=<unavailable>, value=0x0000000000000000, columns=0x00007fc6035fd1d8, timestamp=<unavailable>, status=0x00007fc6035fd250, merge_context=0x00007fc6035fce40, max_covering_tombstone_seq=0x00007fc6035fce90, pinned_iters_mgr=0x00007fc6035fcdf0, value_found=0x0000000000000000, key_exists=0x0000000000000000, seq=0x0000000000000000, callback=0x0000000000000000, is_blob=0x0000000000000000, do_merge=<unavailable>) at version_set.cc:2492
frame #12: 0x000000000051e245 db_stress`rocksdb::DBImpl::GetImpl(this=0x00007fc637a86000, read_options=0x00007fc6035fcf60, key=<unavailable>, get_impl_options=0x00007fc6035fd000) at db_impl.cc:2408
frame #13: 0x000000000050cec2 db_stress`rocksdb::DBImpl::GetEntity(this=0x00007fc637a86000, _read_options=<unavailable>, column_family=<unavailable>, key=0x00007fc6035fd3c8, columns=0x00007fc6035fd1d8) at db_impl.cc:2109
frame #14: 0x000000000074f688 db_stress`rocksdb::(anonymous namespace)::MemTableInserter::MergeCF(this=0x00007fc6035fd450, column_family_id=2, key=0x00007fc6035fd3c8, value=0x00007fc6035fd3a0) at write_batch.cc:2656
frame #15: 0x00000000007476fc db_stress`rocksdb::WriteBatchInternal::Iterate(wb=0x00007fc6035fe698, handler=0x00007fc6035fd450, begin=12, end=<unavailable>) at write_batch.cc:607
frame #16: 0x000000000074d7dd db_stress`rocksdb::WriteBatchInternal::InsertInto(rocksdb::WriteThread::WriteGroup&, unsigned long, rocksdb::ColumnFamilyMemTables*, rocksdb::FlushScheduler*, rocksdb::TrimHistoryScheduler*, bool, unsigned long, rocksdb::DB*, bool, bool, bool) [inlined] rocksdb::WriteBatch::Iterate(this=<unavailable>, handler=0x00007fc6035fd450) const at write_batch.cc:505
frame #17: 0x000000000074d77b db_stress`rocksdb::WriteBatchInternal::InsertInto(write_group=<unavailable>, sequence=<unavailable>, memtables=<unavailable>, flush_scheduler=<unavailable>, trim_history_scheduler=<unavailable>, ignore_missing_column_families=<unavailable>, recovery_log_number=0, db=0x00007fc637a86000, concurrent_memtable_writes=<unavailable>, seq_per_batch=false, batch_per_txn=<unavailable>) at write_batch.cc:3084
frame #18: 0x0000000000631d77 db_stress`rocksdb::DBImpl::PipelinedWriteImpl(this=0x00007fc637a86000, write_options=<unavailable>, my_batch=0x00007fc6035fe698, callback=0x0000000000000000, log_used=<unavailable>, log_ref=0, disable_memtable=<unavailable>, seq_used=0x0000000000000000) at db_impl_write.cc:807
frame #19: 0x000000000062ceeb db_stress`rocksdb::DBImpl::WriteImpl(this=<unavailable>, write_options=<unavailable>, my_batch=0x00007fc6035fe698, callback=0x0000000000000000, log_used=<unavailable>, log_ref=0, disable_memtable=<unavailable>, seq_used=0x0000000000000000, batch_cnt=0, pre_release_callback=0x0000000000000000, post_memtable_callback=0x0000000000000000) at db_impl_write.cc:312
frame #20: 0x000000000062c8ec db_stress`rocksdb::DBImpl::Write(this=0x00007fc637a86000, write_options=0x00007fc6035feca8, my_batch=0x00007fc6035fe698) at db_impl_write.cc:157
frame #21: 0x000000000062b847 db_stress`rocksdb::DB::Merge(this=0x00007fc637a86000, opt=0x00007fc6035feca8, column_family=0x00007fc6370bf140, key=0x00007fc6035fe8d8, value=0x00007fc6035fe830) at db_impl_write.cc:2544
frame #22: 0x000000000062b6ef db_stress`rocksdb::DBImpl::Merge(this=0x00007fc637a86000, o=<unavailable>, column_family=0x00007fc6370bf140, key=0x00007fc6035fe8d8, val=0x00007fc6035fe830) at db_impl_write.cc:72
frame #23: 0x00000000004d6397 db_stress`rocksdb::NonBatchedOpsStressTest::TestPut(this=0x00007fc637041000, thread=0x00007fc6370dbc00, write_opts=0x00007fc6035feca8, read_opts=0x00007fc6035fe9c8, rand_column_families=<unavailable>, rand_keys=size=1, value={P\xe9_\x03\xc6\x7f\0\0}) at no_batched_ops_stress.cc:1317
frame #24: 0x000000000049361d db_stress`rocksdb::StressTest::OperateDb(this=0x00007fc637041000, thread=0x00007fc6370dbc00) at db_stress_test_base.cc:1148
...

facebook-github-bot · 2024-03-20T21:48:47Z

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

facebook-github-bot · 2024-03-20T23:48:37Z

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

facebook-github-bot · 2024-03-20T23:49:31Z

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

facebook-github-bot · 2024-03-20T23:51:05Z

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

facebook-github-bot · 2024-03-20T23:56:39Z

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

facebook-github-bot · 2024-03-21T00:00:36Z

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

facebook-github-bot · 2024-03-21T17:20:30Z

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

facebook-github-bot · 2024-03-21T17:20:49Z

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

ltamasi

Thanks so much for the fix (or rather, fixes) @ajkr! LGTM 👍👍

One thing I'm wondering about if there are more issues related to kBlockCacheTier reads in general - for one, there still seems to be a case in TableCache::MultiGet where an incomplete status is apparently swallowed and turned into OK.

ajkr · 2024-03-21T19:18:18Z

Thanks for the review!

Good point, we should add more kBlockCacheTier testing for other kinds of reads. These findings (this and #12443) simply came from using kBlockCacheTier for max_successive_merges, which only does single-key point lookup.

facebook-github-bot · 2024-03-21T19:43:30Z

@ajkr merged this pull request in bf98dcf.

tedyu · 2024-03-21T22:24:31Z

table/cuckoo/cuckoo_table_reader.cc

          }
          bool dont_care __attribute__((__unused__));
-          get_context->SaveValue(found_ikey, value, &dont_care);
+          get_context->SaveValue(found_ikey, value, &dont_care, &s);


Should we check the return value from SaveValue() ?

I haven't thought about it. Do you have an explanation of what can go wrong without it and/or proposal of what to do with it?

I think we should check the return value as is done in the vicinity of this code.

tedyu · 2024-03-21T22:25:25Z

table/get_context.cc


-    get_context->SaveValue(ikey, value, &dont_care, value_pinner);
+    Status read_status;
+    get_context->SaveValue(ikey, value, &dont_care, &read_status, value_pinner);


Should we check the return value from SaveValue() ?

facebook-github-bot added the CLA Signed label Mar 20, 2024

ajkr added 3 commits March 20, 2024 16:48

Fix kBlockCacheTier read for on-disk blob value

c50789a

replayGetContextLog status handling

e0c31bf

GetFromRowCache status handling

54e2b81

ajkr force-pushed the fix-inmem-get-blob-val branch from 495ee51 to 4022b69 Compare March 20, 2024 23:48

repro test case

98894cd

ajkr force-pushed the fix-inmem-get-blob-val branch from 4022b69 to 98894cd Compare March 20, 2024 23:49

use CO_RETURN

1466dfb

release note

9310c93

ajkr changed the title ~~Fix kBlockCacheTier read for on-disk blob value~~ Fix kBlockCacheTier read when merge-chain base value is in a blob file Mar 21, 2024

ajkr requested a review from ltamasi March 21, 2024 00:01

try to satisfy linter

542c5fe

ltamasi approved these changes Mar 21, 2024

View reviewed changes

facebook-github-bot closed this in bf98dcf Mar 21, 2024

facebook-github-bot added the Merged label Mar 21, 2024

tedyu reviewed Mar 21, 2024

View reviewed changes

Conversation

ajkr commented Mar 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented Mar 20, 2024

Uh oh!

facebook-github-bot commented Mar 20, 2024

Uh oh!

facebook-github-bot commented Mar 20, 2024

Uh oh!

facebook-github-bot commented Mar 20, 2024

Uh oh!

facebook-github-bot commented Mar 20, 2024

Uh oh!

facebook-github-bot commented Mar 21, 2024

Uh oh!

facebook-github-bot commented Mar 21, 2024

Uh oh!

facebook-github-bot commented Mar 21, 2024

Uh oh!

ltamasi left a comment

Choose a reason for hiding this comment

Uh oh!

ajkr commented Mar 21, 2024

Uh oh!

facebook-github-bot commented Mar 21, 2024

Uh oh!

tedyu Mar 21, 2024

Choose a reason for hiding this comment

Uh oh!

ajkr Mar 21, 2024

Choose a reason for hiding this comment

Uh oh!

tedyu Mar 21, 2024

Choose a reason for hiding this comment

Uh oh!

tedyu Mar 21, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ajkr commented Mar 20, 2024 •

edited

Loading