Fix kBlockCacheTier read when merge-chain base value is in a blob file #12462

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

@@ -186,7 +186,10 @@ Status CuckooTableReader::Get(const ReadOptions& /*readOptions*/,
            return s;
          }
          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

@@ -610,8 +610,13 @@ void replayGetContextLog(const Slice& replay_log, const Slice& user_key,

    (void)ret;

-    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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix kBlockCacheTier read when merge-chain base value is in a blob file #12462

Fix kBlockCacheTier read when merge-chain base value is in a blob file #12462

ajkr commented Mar 20, 2024 •

edited

Loading

facebook-github-bot commented Mar 20, 2024

facebook-github-bot commented Mar 20, 2024

facebook-github-bot commented Mar 20, 2024

facebook-github-bot commented Mar 20, 2024

facebook-github-bot commented Mar 20, 2024

facebook-github-bot commented Mar 21, 2024

facebook-github-bot commented Mar 21, 2024

facebook-github-bot commented Mar 21, 2024

ltamasi left a comment

ajkr commented Mar 21, 2024

facebook-github-bot commented Mar 21, 2024

tedyu Mar 21, 2024

ajkr Mar 21, 2024

tedyu Mar 21, 2024

tedyu Mar 21, 2024

Fix kBlockCacheTier read when merge-chain base value is in a blob file #12462

Fix kBlockCacheTier read when merge-chain base value is in a blob file #12462

Conversation

ajkr commented Mar 20, 2024 • edited Loading

facebook-github-bot commented Mar 20, 2024

facebook-github-bot commented Mar 20, 2024

facebook-github-bot commented Mar 20, 2024

facebook-github-bot commented Mar 20, 2024

facebook-github-bot commented Mar 20, 2024

facebook-github-bot commented Mar 21, 2024

facebook-github-bot commented Mar 21, 2024

facebook-github-bot commented Mar 21, 2024

ltamasi left a comment

Choose a reason for hiding this comment

ajkr commented Mar 21, 2024

facebook-github-bot commented Mar 21, 2024

tedyu Mar 21, 2024

Choose a reason for hiding this comment

ajkr Mar 21, 2024

Choose a reason for hiding this comment

tedyu Mar 21, 2024

Choose a reason for hiding this comment

tedyu Mar 21, 2024

Choose a reason for hiding this comment

ajkr commented Mar 20, 2024 •

edited

Loading