On Thu, Dec 22, 2022 at 4:05 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Wed, Dec 21, 2022 at 7:25 PM Masahiko Sawada <sawada.m...@gmail.com> wrote: > > > > On Wed, Dec 21, 2022 at 10:14 PM shiy.f...@fujitsu.com > > <shiy.f...@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
