Thread: Re: Update maintenance_work_mem/autovacuum_work_mem to reflect the 1GB limitation with VACUUM

On Fri, 21 May 2021 at 03:52, Laurenz Albe <laurenz.albe@cybertec.at> wrote:
> Just sending a reply to -hackers so I can add it to the commitfest.

I had a look at the patch in [1] and I find it a bit weird that we'd
write the following about autovacuum_work_mem in our docs:

+       <para>
+        Note that <command>VACUUM</command> has a hard-coded limit of 1GB
+        for the amount of memory used, so setting
+        <varname>autovacuum_work_mem</varname> higher than that has no effect.
+       </para>

Since that setting is *only* used for auto vacuum, why don't we just
limit the range of the GUC to 1GB?

Of course, it wouldn't be wise to backpatch the reduced limit of
autovacuum_work_mem as it might upset people who have higher values in
their postgresql.conf when their database fails to restart after an
upgrade. I think what might be best is just to reduce the limit in
master and apply the doc patch for just maintenance_work_mem in all
supported versions. We could just ignore doing anything with
autovacuum_work_mem in the back branches and put it down to a
historical mistake that can't easily be fixed now.

I've attached what I came up with.

What do you think?

[1] https://www.postgresql.org/message-id/514fe5ce4714b7b33cb0a611f0c7b72df413bef5.camel%40cybertec.at

On Fri, 2021-07-02 at 23:31 +1200, David Rowley wrote:
> I had a look at the patch in [1] and I find it a bit weird that we'd
> write the following about autovacuum_work_mem in our docs:
> 
> +       <para>
> +        Note that <command>VACUUM</command> has a hard-coded limit of 1GB
> +        for the amount of memory used, so setting
> +        <varname>autovacuum_work_mem</varname> higher than that has no effect.
> +       </para>
> 
> Since that setting is *only* used for auto vacuum, why don't we just
> limit the range of the GUC to 1GB?
> 
> Of course, it wouldn't be wise to backpatch the reduced limit of
> autovacuum_work_mem as it might upset people who have higher values in
> their postgresql.conf when their database fails to restart after an
> upgrade. I think what might be best is just to reduce the limit in
> master and apply the doc patch for just maintenance_work_mem in all
> supported versions. We could just ignore doing anything with
> autovacuum_work_mem in the back branches and put it down to a
> historical mistake that can't easily be fixed now.
> 
> I've attached what I came up with.
> 
> What do you think?
> 
> [1] https://www.postgresql.org/message-id/514fe5ce4714b7b33cb0a611f0c7b72df413bef5.camel%40cybertec.at

I think that is much better.
I am fine with that patch.

Yours,
Laurenz Albe

On Sat, 3 Jul 2021 at 00:40, Laurenz Albe <laurenz.albe@cybertec.at> wrote:
>
> On Fri, 2021-07-02 at 23:31 +1200, David Rowley wrote:
> > I had a look at the patch in [1] and I find it a bit weird that we'd
> > write the following about autovacuum_work_mem in our docs:
> >
> > +       <para>
> > +        Note that <command>VACUUM</command> has a hard-coded limit of 1GB
> > +        for the amount of memory used, so setting
> > +        <varname>autovacuum_work_mem</varname> higher than that has no effect.
> > +       </para>
> >
> > Since that setting is *only* used for auto vacuum, why don't we just
> > limit the range of the GUC to 1GB?
> >
> > Of course, it wouldn't be wise to backpatch the reduced limit of
> > autovacuum_work_mem as it might upset people who have higher values in
> > their postgresql.conf when their database fails to restart after an
> > upgrade. I think what might be best is just to reduce the limit in
> > master and apply the doc patch for just maintenance_work_mem in all
> > supported versions. We could just ignore doing anything with
> > autovacuum_work_mem in the back branches and put it down to a
> > historical mistake that can't easily be fixed now.
> >
>
> I think that is much better.
> I am fine with that patch.

Thanks for looking.  I've pushed the doc fix patch for
maintenance_work_mem and back-patched to 9.6.

I could do with a 2nd opinion about if we should just adjust the
maximum value for the autovacuum_work_mem GUC to 1GB in master.

I'm also not sure if since we'd not backpatch the GUC max value
adjustment if we need to document the upper limit in the manual.

David

On Sun, 4 Jul 2021 at 22:38, David Rowley <dgrowleyml@gmail.com> wrote:
> I could do with a 2nd opinion about if we should just adjust the
> maximum value for the autovacuum_work_mem GUC to 1GB in master.
>
> I'm also not sure if since we'd not backpatch the GUC max value
> adjustment if we need to document the upper limit in the manual.

I was just looking at this again and I see that GIN indexes are able
to use more than 1GB of memory during VACUUM. That discovery makes me
think having the docs say that vacuum cannot use more than 1GB of
memory is at best misleading and more likely just incorrect.

Right now I'm considering if it might just be better to revert
ec34040af and call it quits here.

David

On Wed, Jul 7, 2021 at 8:44 PM David Rowley <dgrowleyml@gmail.com> wrote:
>
> On Sun, 4 Jul 2021 at 22:38, David Rowley <dgrowleyml@gmail.com> wrote:
> > I could do with a 2nd opinion about if we should just adjust the
> > maximum value for the autovacuum_work_mem GUC to 1GB in master.
> >
> > I'm also not sure if since we'd not backpatch the GUC max value
> > adjustment if we need to document the upper limit in the manual.
>
> I was just looking at this again and I see that GIN indexes are able
> to use more than 1GB of memory during VACUUM.

I think you meant that autovacuums can use more than 1GB of memory
during cleaning up a gin pending list (in ginInsertCleanup()). The
description updated by that commit is not true as of now as you
pointed out but IIUC it uses maintenance_work_mem *in addition to* the
same amount memory used by lazy vacuum. This memory usage seems rather
weird to me. Is it worh considering having gin pending list cleanup
use work_mem instead of maintenance_work_mem also in autovacuum cases
like btree indexes do? If we do that, the description will become
true, although we might need to update work_mem section somewhat.

Regards,

--
Masahiko Sawada
EDB:  https://www.enterprisedb.com/

On Wed, 7 Jul 2021 at 23:44, David Rowley <dgrowleyml@gmail.com> wrote:
>
> On Sun, 4 Jul 2021 at 22:38, David Rowley <dgrowleyml@gmail.com> wrote:
> > I could do with a 2nd opinion about if we should just adjust the
> > maximum value for the autovacuum_work_mem GUC to 1GB in master.
> >
> > I'm also not sure if since we'd not backpatch the GUC max value
> > adjustment if we need to document the upper limit in the manual.
>
> I was just looking at this again and I see that GIN indexes are able
> to use more than 1GB of memory during VACUUM. That discovery makes me
> think having the docs say that vacuum cannot use more than 1GB of
> memory is at best misleading and more likely just incorrect.

The attached patch aims to put right where I went wrong with the
documentation about vacuum/autovacuum only using maintainance_work_mem
memory for dead tuple collection.

I plan to push this and backpatch to 9.6 shortly unless there are any
better ideas.

What's in there right now is wrong and I want that fixed before the
cut-off for the next set of bug fix releases.

David

On Mon, 9 Aug 2021 at 14:44, David Rowley <dgrowleyml@gmail.com> wrote:
> I plan to push this and backpatch to 9.6 shortly unless there are any
> better ideas.

I pushed this patch.  I've now marked the entry in the commitfest app
as committed too.

David