Details
-
Bug
-
Resolution: Fixed
-
Major - P3
-
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.