Re: Fix memory counter update in reorderbuffer

On Tue, Aug 6, 2024 at 1:12 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Sat, Aug 3, 2024 at 1:21 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> >
> > I found a bug in the memory counter update in reorderbuffer. It was
> > introduced by commit 5bec1d6bc5e, so pg17 and master are affected.
> >
> > In ReorderBufferCleanupTXN() we zero the transaction size and then
> > free the transaction entry as follows:
> >
> > /* Update the memory counter */
> > ReorderBufferChangeMemoryUpdate(rb, NULL, txn, false, txn->size);
> >
> > /* deallocate */
> > ReorderBufferReturnTXN(rb, txn);
> >
>
> Why do we need to zero the transaction size explicitly? Shouldn't it
> automatically become zero after freeing all the changes?

It will become zero after freeing all the changes. However, since
updating the max-heap when freeing each change could bring some
overhead, we freed the changes without updating the memory counter,
and then zerod it.

>
> > However, if the transaction entry has toast changes we free them in
> > ReorderBufferToastReset() called from ReorderBufferToastReset(),
> >
>
> Here, you mean ReorderBufferToastReset() called from
> ReorderBufferReturnTXN(), right?

Right. Thank you for pointing it out.

> BTW, commit 5bec1d6bc5e also introduced
> ReorderBufferChangeMemoryUpdate() in ReorderBufferTruncateTXN() which
> is also worth considering while fixing the reported problem. It may
> not have the same problem as you have reported but we can consider
> whether setting txn size as zero explicitly is required or not.

The reason why it introduced ReorderBufferChangeMemoryUpdate() is the
same as I mentioned above. And yes, as you mentioned, it doesn't have
the same problem that I reported here.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com