Re: [UNVERIFIED SENDER] Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate()

On 9/7/21 9:11 AM, Drouvot, Bertrand wrote:
>
> On 9/7/21 8:51 AM, Amit Kapila wrote:
>> On Tue, Sep 7, 2021 at 11:33 AM Drouvot, Bertrand 
>> <bdrouvot@amazon.com> wrote:
>>> Hi,
>>>
>>> On 9/7/21 7:58 AM, Dilip Kumar wrote:
>>>
>>>
>>> On Tue, Sep 7, 2021 at 11:10 AM Amit Kapila 
>>> <amit.kapila16@gmail.com> wrote:
>>>
>>>>>> Isn't it better if we use option 2) at all places as then we won't
>>>>>> need any special check inside ReorderBufferChangeMemoryUpdate()?
>>>>>
>>>>> If we want to do this then be careful about 
>>>>> REORDER_BUFFER_CHANGE_INTERNAL_TUPLECID change. Basically, 
>>>>> ReorderBufferChangeMemoryUpdate() ignores this type of change 
>>>>> whereas ReorderBufferChangeSize(), consider at least 
>>>>> sizeof(ReorderBufferChange) bytes to this change.  So if we 
>>>>> compute the size using ReorderBufferChangeSize() outside of 
>>>>> ReorderBufferChangeMemoryUpdate(), then total size will be 
>>>>> different from what we have now.   Logically, we should be 
>>>>> ignoring/asserting REORDER_BUFFER_CHANGE_INTERNAL_TUPLECID in 
>>>>> ReorderBufferChangeSize(), because 
>>>>> ReorderBufferChangeMemoryUpdate() is the only caller for this 
>>>>> function.
>>>>>
>>>> Why can't we simply ignore it in ReorderBufferChangeMemoryUpdate() as
>>>> we are doing now?
>>>
>>> Yeah right, we can actually do that, it doesn't matter even if we 
>>> are passing the size from outside.
>>>
>>> Agree, if no objections, I'll prepare a patch with the modified 
>>> approach of option 2) proposed by Amit (means passing the size from 
>>> the outside in all the cases).
>>>
>> Sounds reasonable. Another point that needs some thought is do we want
>> to backpatch this change till v13?  I am not sure if there is any
>> user-visible bug here but maybe it is still good to fix this in back
>> branches. What do you think?
>
> Yes, +1 to backpatch till v13 "per precaution".
>
> I will first come back with a proposed version for master and once we 
> agree on a final/polished version then I'll do the backpatch ones (if 
> needed).

Please find enclosed patch v2 (for the master branch) implementing the 
modified approach of option 2) proposed by Amit.

Thanks

Bertrand