On Thu, Dec 22, 2022 at 4:05 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Dec 21, 2022 at 7:25 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Wed, Dec 21, 2022 at 10:14 PM shiy.fnst@fujitsu.com
> > <shiy.fnst@fujitsu.com> wrote:
> >
> > The patch looks good to me. Some minor comments are:
> >
> > - * limit, but we might also adapt a more elaborate eviction strategy
> > - for example
> > - * evicting enough transactions to free certain fraction (e.g. 50%)
> > of the memory
> > - * limit.
> > + * limit, but we might also adapt a more elaborate eviction strategy - for
> > + * example evicting enough transactions to free certain fraction (e.g. 50%) of
> > + * the memory limit.
> >
> > This change is not relevant with this feature.
> >
> > ---
> > + if (logical_decoding_mode == LOGICAL_DECODING_MODE_DEFAULT
> > + && rb->size < logical_decoding_work_mem * 1024L)
> >
> > Since we put '&&' before the new line in all other places in
> > reorderbuffer.c, I think it's better to make it consistent. The same
> > is true for the change for while loop in the patch.
> >
>
> I have addressed these comments in the attached. Additionally, I have
> modified the docs and commit messages to make those clear.
Thanks!
> I think
> instead of adding new tests with this patch, it may be better to
> change some of the existing tests related to streaming to use this
> parameter as that will clearly show one of the purposes of this patch.
+1. I think test_decoding/sql/stream.sql and spill.sql are good
candidates and we change logical replication TAP tests in a separate
patch.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
