Re: Fix memory counter update in reorderbuffer

On Wed, Aug 7, 2024 at 7:42 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> 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.
>

I think this should be covered in comments as it is not apparent.

>
> > 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.
>

I checked again and found that ReorderBufferResetTXN() first calls
ReorderBufferTruncateTXN() and then ReorderBufferToastReset(). After
that, it also tries to free spec_insert change which should have the
same problem. So, what saves this path from the same problem?

*
+ /*
+ * Update the memory counter of the transaction, removing it from
+ * the max-heap.
+ */
+ ReorderBufferChangeMemoryUpdate(rb, NULL, txn, false, txn->size);
+ Assert(txn->size == 0);
+
pfree(txn);

Just before freeing the TXN, updating the size looks odd though I
understand the idea is to remove TXN from max_heap. Anyway, let's
first discuss whether the same problem exists in
ReorderBufferResetTXN() code path, and if so, how we want to fix it.

--
With Regards,
Amit Kapila.