Re: PATCH: logical_work_mem and logical streaming of largein-progress transactions - Mailing list pgsql-hackers

On Fri, Sep 27, 2019 at 4:55 PM Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:
>
> On Fri, Sep 27, 2019 at 02:33:32PM +0530, Amit Kapila wrote:
> >On Tue, Jan 9, 2018 at 7:55 AM Peter Eisentraut
> ><peter.eisentraut@2ndquadrant.com> wrote:
> >>
> >> On 1/3/18 14:53, Tomas Vondra wrote:
> >> >> I don't see the need to tie this setting to maintenance_work_mem.
> >> >> maintenance_work_mem is often set to very large values, which could
> >> >> then have undesirable side effects on this use.
> >> >
> >> > Well, we need to pick some default value, and we can either use a fixed
> >> > value (not sure what would be a good default) or tie it to an existing
> >> > GUC. We only really have work_mem and maintenance_work_mem, and the
> >> > walsender process will never use more than one such buffer. Which seems
> >> > to be closer to maintenance_work_mem.
> >> >
> >> > Pretty much any default value can have undesirable side effects.
> >>
> >> Let's just make it an independent setting unless we know any better.  We
> >> don't have a lot of settings that depend on other settings, and the ones
> >> we do have a very specific relationship.
> >>
> >> >> Moreover, the name logical_work_mem makes it sound like it's a logical
> >> >> version of work_mem.  Maybe we could think of another name.
> >> >
> >> > I won't object to a better name, of course. Any proposals?
> >>
> >> logical_decoding_[work_]mem?
> >>
> >
> >Having a separate variable for this can give more flexibility, but
> >OTOH it will add one more knob which user might not have a good idea
> >to set.  What are the problems we see if directly use work_mem for
> >this case?
> >
>
> IMHO it's similar to autovacuum_work_mem - we have an independent
> setting, but most people use it as -1 so we use maintenance_work_mem as
> a default value. I think it makes sense to do the same thing here.
>
> It does ad an extra knob anyway (I don't think we should just use
> maintenance_work_mem directly, the user should have an option to
> override it when needed). But most users will not notice.
>
> FWIW I don't think we should use work_mem, maintenace_work_mem seems
> somewhat more appropriate here (not related to queries, etc.).
>

I have the same concern for using maintenace_work_mem as Peter E.
which is that the value of maintenace_work_mem will generally be
higher which is suitable for its current purpose, but not for the
purpose this patch is using.  AFAIU, at this stage we want a better
memory accounting system for logical decoding and we are not sure what
is a good value for this variable.  So, I think using work_mem or
maintenace_work_mem should serve the purpose.  Later, if we have
requirements from people to have better control over the memory
required for this purpose then we can introduce a new variable.

I understand that currently work_mem is primarily tied with memory
used for query workspaces, but it might be okay to extend it for this
purpose.  Another point is that the default for that sound to be more
appealing for this case.  I can see the argument against it which is
having a separate variable will make the things look clean and give
better control.  So, if we can't convince ourselves for using
work_mem, we can introduce a new guc variable and keep the default as
4MB or work_mem.

I feel it is always tempting to introduce a new guc for the different
tasks unless there is an exact match, but OTOH, having lesser guc's
has its own advantage which is that people don't have to bother about
a new setting which they need to tune and especially for which they
can't decide with ease.  I am not telling that we should not introduce
new guc when it is required, but just to give more thought before
doing so.

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