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

On Mon, 2021-05-03 at 13:48 -0300, Martín Marqués wrote:

We should add a line that indicates that there is a limitation (that
should be IMO, backported to documentation of earlier versions as it
affects all supported versions), at least until such limitation is
lifted.

Here is a patch for that,

Yours,
Laurenz Albe

On Wed, 2021-05-05 at 12:03 +0200, Laurenz Albe wrote:

On Mon, 2021-05-03 at 13:48 -0300, Martín Marqués wrote:

We should add a line that indicates that there is a limitation (that
should be IMO, backported to documentation of earlier versions as it
affects all supported versions), at least until such limitation is
lifted.

Here is a patch for that

Just sending a reply to -hackers so I can add it to the commitfest.

Yours,
Laurenz Albe

s/Node/Note/

Other than that, +1 to the patch and +1 to backpatching.

The new status of this patch is: Waiting on Author

On Wed, May 5, 2021 at 7:03 PM Laurenz Albe <laurenz.albe@cybertec.at> wrote:

On Mon, 2021-05-03 at 13:48 -0300, Martín Marqués wrote:

We should add a line that indicates that there is a limitation (that
should be IMO, backported to documentation of earlier versions as it
affects all supported versions), at least until such limitation is
lifted.

Here is a patch for that,

The patch adds the description in the autovacuum_work_mem section.
Isn't it better to add it in mantenance_work section or VACUUM command
section since this limitation is not only for autovacuum?

Regards,

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

On Wed, 2021-06-02 at 18:16 +0900, Masahiko Sawada wrote:

We should add a line that indicates that there is a limitation (that
should be IMO, backported to documentation of earlier versions as it
affects all supported versions), at least until such limitation is
lifted.

Here is a patch for that,

The patch adds the description in the autovacuum_work_mem section.
Isn't it better to add it in mantenance_work section or VACUUM command
section since this limitation is not only for autovacuum?

You are right; theoretically, the correct place to document that
would be the VACUUM documentation. But I guess that most people who
are curious about VACUUM's memory usage will read the documentation for
"maintenance_work_mem" or "autovacuum_work_mem".

I have fixed a typo and added a similar paragraph to "maintenance_work_mem".

Yours,
Laurenz Albe

Should we say "currently has"?

The following review has been posted through the commitfest application:
make installcheck-world: not tested
Implements feature: not tested
Spec compliant: not tested
Documentation: not tested

Latest patch looks fine to me, to be clear.

The new status of this patch is: Ready for Committer

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]/messages/by-id/514fe5ce4714b7b33cb0a611f0c7b72df413bef5.camel@cybertec.at 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]: /messages/by-id/514fe5ce4714b7b33cb0a611f0c7b72df413bef5.camel@cybertec.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] /messages/by-id/514fe5ce4714b7b33cb0a611f0c7b72df413bef5.camel@cybertec.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