Re: PATCH: logical_work_mem and logical streaming of large in-progress transactions

On Mon, Feb 10, 2020 at 1:52 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Fri, Feb 7, 2020 at 4:18 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> >
> > On Wed, Feb 5, 2020 at 4:05 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > >
> > > On Wed, Feb 5, 2020 at 9:46 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> > > >
> > > > Fixed in the latest version sent upthread.
> > > >
> > >
> > > Okay, thanks. I haven't looked at the latest version of patch series
> > > as I was reviewing the previous version and I think all of these
> > > comments are in the patch which is not modified. Here are my
> > > comments:
> > >
> > > I think we don't need to maintain
> > > v8-0007-Support-logical_decoding_work_mem-set-from-create as per
> > > discussion in one of the above emails [1] as its usage is not clear.
> > >
> > > v8-0008-Add-support-for-streaming-to-built-in-replication
> > > 1.
> > > - information. The allowed options are <literal>slot_name</literal> and
> > > - <literal>synchronous_commit</literal>
> > > + information. The allowed options are <literal>slot_name</literal>,
> > > + <literal>synchronous_commit</literal>, <literal>work_mem</literal>
> > > + and <literal>streaming</literal>.
> > >
> > > As per the discussion above [1], I don't think we need work_mem here.
> > > You might want to remove the other usage from the patch as well.
> >
> > After putting more thought on this it appears that there could be some
> > use cases for setting the work_mem from the subscription, Assume a
> > case where data are coming from two different origins and based on the
> > origin ids different slots might collect different type of changes,
> > So isn't it good to have different work_mem for different slots? I am
> > not saying that the current way of implementing is the best one but
> > that we can improve. First, we need to decide whether we have a use
> > case for this or not.
> >
>
> That is the whole point. I don't see a very clear usage of this and
> neither did anybody explained clearly how it will be useful. I am not
> denying that what you are describing has no use, but as you said we
> might need to invent an entirely new way even if we have such a use.
> I think it is better to avoid the requirements which are not essential
> for this patch.

Ok, I will include this change in the next patch set.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com