On Tue, Sep 7, 2021 at 11:33 AM Drouvot, Bertrand <bdrou...@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.kapil...@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? -- With Regards, Amit Kapila.
