Re: Fix memory counter update in reorderbuffer

Hi,

On Fri, Aug 16, 2024 at 12:22 AM Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
>
> On Wed, 7 Aug 2024 at 11:48, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > 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?
>
> I tried testing this scenario and I was able to reproduce the crash in
> HEAD with this scenario. I have created a patch for the testcase.
> I also tested the same scenario with the latest patch shared by
> Sawada-san in [1]. And confirm that this resolves the issue.

Thank you for testing the patch!

I'm slightly hesitant to add a test under src/test/subscription since
it's a bug in ReorderBuffer and not specific to logical replication.
If we reasonably cannot add a test under contrib/test_decoding, I'm
okay with adding it under src/test/subscription.

I've attached the updated patch with the commit message (but without a
test case for now).

Regards,

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