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