storage: IngestExternalFiles returns "external files have corrupted keys" on Mac

[jeffrey-xiao]

Ingestion of SSTs with range deletion tombstones with this patch returns "external files have corrupted keys" on Macs, but not on Linux. For example, TestImportIntoCSV consistently fails with #39689. Removing the line where we set read_options fixes this error.

cc @nvanbenschoten @ajkr @tbg

[nvanbenschoten]

IngestRangeDeletionTombstoneWithGlobalSeqno, the test added in that patch, passes on Mac.

[nvanbenschoten]

I think this is a bug with pinning keys in the FragmentedRangeTombstoneIterator. This diff appears to fix the issue:

diff --git a/db/range_tombstone_fragmenter.h b/db/range_tombstone_fragmenter.h
index a0b77b677..dda1f303d 100644
--- a/db/range_tombstone_fragmenter.h
+++ b/db/range_tombstone_fragmenter.h
@@ -220,8 +220,7 @@ class FragmentedRangeTombstoneIterator : public InternalIterator {
   };

   void MaybePinKey() const {
-    if (pos_ != tombstones_->end() && seq_pos_ != tombstones_->seq_end() &&
-        (pinned_pos_ != pos_ || pinned_seq_pos_ != seq_pos_)) {
+    if (true) {
       current_start_key_.Set(pos_->start_key, *seq_pos_, kTypeRangeDeletion);
       pinned_pos_ = pos_;
       pinned_seq_pos_ = seq_pos_;

[ajkr]

I wonder if the seq_pos_ != tombstones_->seq_end() is failing only in ingestion preparation because the tombstones all have seqnum zero.

edit: seems doubtful. I wonder which of the conditions are returning an unexpected result.

[nvanbenschoten]

Good question. It looks like pinned_pos_ != pos_ || pinned_seq_pos_ != seq_pos_ is evaluting to false.

[ajkr]

Huh, could it be because pinned_pos_ and pinned_seq_pos_ are uninitialized? I really cannot think of any reason why that condition would evaluate to false when our sequence of calls is SeekToFirst(), key(), Next(), key(), Next(), key(), ... seq_pos_ at least should be strictly increasing between consecutive calls to key() -> MaybePinKey(), so it should always differ from pinned_seq_pos_.

If we initialize those in the FragmentedRangeTombstoneIterator constructor to the end() of their respective containers, any chance it changes the outcome?

[nvanbenschoten]

Heh yep, this does the trick:

diff --git a/db/range_tombstone_fragmenter.cc b/db/range_tombstone_fragmenter.cc
index e3eb18908..39126737b 100644
--- a/db/range_tombstone_fragmenter.cc
+++ b/db/range_tombstone_fragmenter.cc
@@ -224,7 +224,9 @@ FragmentedRangeTombstoneIterator::FragmentedRangeTombstoneIterator(
       ucmp_(icmp.user_comparator()),
       tombstones_(tombstones),
       upper_bound_(_upper_bound),
-      lower_bound_(_lower_bound) {
+      lower_bound_(_lower_bound),
+      pinned_pos_(tombstones->end()),
+      pinned_seq_pos_(tombstones->seq_end()) {
   assert(tombstones_ != nullptr);
   Invalidate();
 }
@@ -240,7 +242,9 @@ FragmentedRangeTombstoneIterator::FragmentedRangeTombstoneIterator(
       tombstones_ref_(tombstones),
       tombstones_(tombstones_ref_.get()),
       upper_bound_(_upper_bound),
-      lower_bound_(_lower_bound) {
+      lower_bound_(_lower_bound),
+      pinned_pos_(tombstones->end()),
+      pinned_seq_pos_(tombstones->seq_end()) {
   assert(tombstones_ != nullptr);
   Invalidate();
 }

Did Clang change its treatment of uninitialized member variables across the version we use on Mac and Linux?

Anyway, I'm out tomorrow, so free to turn that into a PR and use it to revert the revert of the reverted revert of Jeffrey's original PR. Let's get that in for good!

[petermattis]

Did Clang change its treatment of uninitialized member variables across the version we use on Mac and Linux?

Probably not as that stuff has been in the C++ language spec for a while. This could be due to differences in memory allocator patterns causing the members to be accidentally initialized to zero on Linux for this test, but full of garbage on Mac OS X. If my memory serves me, it is invalid to expect an STL iterator to be initialized to any value: you have to initialize it explicitly.

[petermattis]

If I'm reading this correctly, vector<T>::const_iterator is a const T*. That means there is no default constructor and thus these members were definitely uninitialized.

Note that a vector<T>::iterator is invalidated by any mutation to the vector such as push_back or erase. I think there is another bug lurking here that we need to invalidate {pinned_pos_,pinned_seq_pos_ whenever tombstones_ is changed.

[petermattis]

Note that a vector::iterator is invalidated by any mutation to the vector such as push_back or erase. I think there is another bug lurking here that we need to invalidate {pinned_pos_,pinned_seq_pos_ whenever tombstones_ is changed.

Oh, I don't think tombstones_ ever changes. So this comment can be ignored.

[jeffrey-xiao]

Closed by cockroachdb/rocksdb#44.

[ajkr]

Probably not as that stuff has been in the C++ language spec for a while. This could be due to differences in memory allocator patterns causing the members to be accidentally initialized to zero on Linux for this test, but full of garbage on Mac OS X.

@danhhz was telling me this week about how uninitialized variables can be treated differently by the compiler from an initialized variable in that the memory contents of an uninitialized variable might be ignored when reading it. Actually that conversation is what led me to this guess - if garbage data or zero data were read for these uninitialized iterators, it'd be highly unlikely to make pinned_pos_ != pos_ || pinned_seq_pos_ != seq_pos_ to be false.

[ajkr]

BTW, how about reusing this issue to track upstreaming the fix?

[jeffrey-xiao]

Upstream fix is facebook/rocksdb#5720.