Re: [BUG] Failed Assertion in ReorderBufferChangeMemoryUpdate()

On Tue, Sep 7, 2021 at 8:38 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:

> On Mon, Sep 6, 2021 at 9:14 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> >
> > On Mon, Sep 6, 2021 at 8:54 PM Drouvot, Bertrand <bdrouvot(at)amazon(dot)com>
> wrote:
> >>
> >> Thanks for your feedback!
> >>
> >> That seems indeed more logical, so I see 3 options to do so:
> >>
> >> 1) Add a new API say ReorderBufferChangeMemorySubstractSize() (with a
> Size as one parameter) and make use of it in ReorderBufferToastReplace()
> >>
> >> 2) Add a new "Size" parameter to ReorderBufferChangeMemoryUpdate(), so
> that if this parameter is > 0 then it would be used instead of "sz =
> ReorderBufferChangeSize(change)"
> >>
> >> 3) Do the substraction directly into ReorderBufferToastReplace()
> without any API
> >>
> >> I'm inclined to go for option 2), what do you think?
> >
>
> 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.

> > Yet another option could be to create a new API say
> ReorderBufferReplaceChangeMemoryUpdate(), which takes, 2 parameters,
> oldchange, and newchange as inputs, it will compute the difference and
> add/subtract that size. Logically, that is what we are actually trying to
> do right? i.e. replacing old change with the new change.
> >
>
> Note that in ReorderBufferToastReplace(), the new tuple is replaced in
> change so by the time we want to do this computation oldchange won't
> be preserved.
>

Right

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com