Uploaded image for project: 'Core Server'
  1. Core Server
  2. SERVER-56061

Mark MaterializedRow move constructor and destructor as noexcept

    XMLWordPrintableJSON

Details

    • Icon: Bug Bug
    • Resolution: Fixed
    • Icon: Major - P3 Major - P3
    • 5.0.0-rc0
    • None
    • None
    • None
    • Fully Compatible
    • ALL
    • Query Execution 2021-04-19, Query Execution 2021-05-03

    Description

      The repro below shows how accessor, pointing to the element of spool buffer, can become invalid after new element is added to the spool buffer.

      const StringData stringBody = "Test string contents";
       
      // SpoolBuffer is a vector of MaterializedRow objects.
      SpoolBuffer spool;
       
      auto pushStringRow = [&](){
          value::MaterializedRow row{1};
          auto [tag, val] = value::makeBigString(stringBody);
          row.reset(0, true, tag, val);
          spool.emplace_back(std::move(row));
      };
       
      pushStringRow();
       
      // Create accessor to the first MaterializedRow and get view of its value.
      value::MaterializedRowAccessor<SpoolBuffer> accessor{spool, 0, 0};
      auto [tag, val] = accessor.getViewOfValue();
       
      // This assert passes.
      ASSERT_EQUALS(value::getStringView(tag, val), stringBody);
       
      // This push results in reallocation of vector's elements. During reallocation, copy constructor is
      // used instead of a move constructor. This makes MaterializedRow to free memory [tag, val] pair is
      // pointing to, making the MaterializedRowAccessor invalid.
      pushStringRow();
       
      // This assert fails.
      ASSERT_EQUALS(value::getStringView(tag, val), stringBody);
      

      This was discovered during manual inspection of values in the following SBE plan:

      [2] nlj [] [s2]
          left
              [1] filter {isRecordId (s2)}
              [1] lspool sp1 [s2] {! isRecordId (s2)}
              [1] union [s2] [
                  [s3] [1] project [s3 = KS(2B322B020104)]
                  [1] limit 1
                  [1] coscan ,
                  [s8] [1] nlj [] [s6]
                      left
                          [1] sspool sp1 [s6]
                      right
                          [1] chkbounds s4 s5 s8
                          [1] nlj [] [s7]
                              left
                                  [1] project [s7 = s6]
                                  [1] limit 1
                                  [1] coscan
                              right
                                  [1] ixseek s7 s4 s5 [] @"..." @"age_1_rating_1" true
       
       
       
       
             ]
          right
              [2] limit 1
              [2] seek s2 s9 s10 [] @"..." true
      

      After lspool stage added a new element to the spool buffer, reallocation of buffer happened and value behind s7 became invalid.

       This happened because std::vector has strong exception guarantees for push_back method. Section 26.3.11.5 of n4659 C++17 standard says that

      *If an exception is thrown while inserting a single element at the end and T is CopyInsertable or is_­nothrow_­move_­constructible_­v<T> is true, there are no effects.*

      We know that type MaterializedRow is CopyInsertable, so std::vector::push_back must guarantee that even if exception happens during the insertion, there are no effects.

      We also know that is_­nothrow_­move_­constructible_­v<MaterializedRow> is false, meaning that move constructor of MaterializedRow can potentially throw. So, the implementation of std::vector::push_back cannot use it because if this constructor will throw, object from which we move can be in an inconsistent state, which is an effect.

      As a result, std::vector::push_back uses copy constructor of MaterializedRow. This forces MaterializedRow to delete old values and create new ones, making accessor to the old values invalid.

      To avoid this problem, we can mark MaterializedRow's move constructor and destructor as noexcept. After this change, is_­nothrow_­move_­constructible_­v<MaterializedRow> becomes true and move constructor is used by std::vector::push_back.

      Attachments

        Activity

          People

            nikita.lapkov@mongodb.com Nikita Lapkov (Inactive)
            nikita.lapkov@mongodb.com Nikita Lapkov (Inactive)
            Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved: