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

On Sun, Sep 29, 2019 at 12:39 AM Tomas Vondra
<tomas(dot)vondra(at)2ndquadrant(dot)com> wrote:
>
> On Sat, Sep 28, 2019 at 01:36:46PM +0530, Amit Kapila wrote:
> >On Fri, Sep 27, 2019 at 4:55 PM Tomas Vondra
> ><tomas(dot)vondra(at)2ndquadrant(dot)com> wrote:
>
> I do think having a separate GUC is a must, irrespectedly of what other
> GUC (if any) is used as a default. You're right the maintenance_work_mem
> value might be too high (e.g. in cases with many subscriptions), but the
> same issue applies to work_mem - there's no guarantee work_mem is lower
> than maintenance_work_mem, and in analytics databases it may be set very
> high. So work_mem does not really solve the issue
>
> IMHO we can't really do without a new GUC. It's not difficult to create
> examples that would benefit from small/large memory limit, depending on
> the number of subscriptions etc.
>
> I do however agree the GUC does not have to be tied to any existing one,
> it was just an attempt to use a more sensible default value. I do think
> m_w_m would be fine, but I can live with using an explicit value.
>
> So that's what I did in the attached patch - I've renamed the GUC to
> logical_decoding_work_mem, detached it from m_w_m and set the default to
> 64MB (i.e. the same default as m_w_m).

Fair enough, let's not argue more on this unless someone else wants to
share his opinion.

> It should also fix all the issues
> from the recent reviews (at least I believe so).
>

Have you given any thought on creating a test case for this patch? I
think you also told that you will test some worst-case scenarios and
report the numbers so that we are convinced that the current eviction
algorithm is good.

> I've realized that one of the subsequent patches allows overriding the
> limit for individual subscriptions (in the CREATE SUBSCRIPTION command).
> I think it'd be good to move this bit forward, but I think it can be
> done in a separate patch.
>

Yeah, it is better to deal it separately as I am also not entirely
convinced at this stage about this parameter. I have mentioned the
same in the previous email as well.

While glancing through the changes, I noticed a small thing:
+#logical_decoding_work_mem = 64MB # min 1MB, or -1 to use maintenance_work_mem

I guess this need to be updated.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com