Re: Fix memory counter update in reorderbuffer

On Tue, Aug 20, 2024 at 9:29 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Aug 20, 2024 at 5:55 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Fri, Aug 16, 2024 at 12:22 AM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
> > >
> >
> > 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.
> >
>
> Sounds like a reasonable approach.

I've managed to reproduce this issue without logical replication based
on the test shared by Shlok Kyal.

>
> > I've attached the updated patch with the commit message (but without a
> > test case for now).
> >
>
> The patch LGTM except for one minor comment.
>
> + /* All changes must be returned */
> + Assert(txn->size == 0);
>
> Isn't it better to say: "All changes must be deallocated." in the above comment?

Agreed.

I've updated the updated patch with regression tests.

Regards,

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