[SERVER-56061] Mark MaterializedRow move constructor and destructor as noexcept Created: 13/Apr/21  Updated: 29/Oct/23  Resolved: 19/Apr/21

Status: Closed
Project: Core Server
Component/s: None
Affects Version/s: None
Fix Version/s: 5.0.0-rc0

Type: Bug Priority: Major - P3
Reporter: Nikita Lapkov (Inactive) Assignee: Nikita Lapkov (Inactive)
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Backwards Compatibility: Fully Compatible
Operating System: ALL
Sprint: Query Execution 2021-04-19, Query Execution 2021-05-03
Participants:

 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.



 Comments   
Comment by Githook User [ 19/Apr/21 ]

Author:

{'name': 'Nikita Lapkov', 'email': 'nikita.lapkov@mongodb.com', 'username': 'laplab'}

Message: SERVER-56061 Mark MaterializedRow move constructor and destructor as noexcept
Branch: master
https://github.com/mongodb/mongo/commit/27b3c5b44644f5ba683c9ffe56f588f6382711a0

Generated at Thu Feb 08 05:38:12 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.