Re: review: autovacuum_work_mem

Please reply to the original thread in future (even if the Reply-to
Message-ID is the same, I see this as a separate thread).

On Fri, Nov 15, 2013 at 5:37 PM, Nigel Heron <nheron@querymetrics.com> wrote:
> On Sun, Oct 20, 2013 at 7:21 AM, Magnus Hagander <magnus@hagander.net> wrote:
>> On Sun, Oct 20, 2013 at 2:22 AM, Peter Geoghegan <pg@heroku.com> wrote:
>
>>> It seemed neater to me to create a new flag, so that in principle any
>>> vacuum() code path can request autovacuum_work_mem, rather than having
>>> lazyvacuum.c code call IsAutoVacuumWorkerProcess() for the same
>>> purpose. To date, that's only been done within vacuumlazy.c for things
>>> like logging.

> I agree with Magnus here, calling IsAutoVacuumWorkerProcess() seems
> cleaner than the new flag and the function parameter changes.

Well, what I did was analogous to the existing coding for
VACOPT_NOWAIT. But it's hardly worth discussing much further. Patch
updated along these lines.

>> Also, isn't this quite confusing:
>> + # Note:  autovacuum only prefers autovacuum_work_mem over maintenance_work_mem
>> + #autovacuum_work_mem = -1 # min 1MB, or -1 to disable
>>
>> Where does the "only" come from? And we don't really use the term
>> "prefers over" anywhere else there.

"Only" could be replaced by "merely" here. In any case, a more
succinct wording is now used.

> here's my review of the v1 patch,
>
> patch features tested:
> - regular VACUUM * commands ignore autovacuum_work_mem.
> - if autovacuum_work_mem = -1 then maintenance_work_mem is used by autovacuum.
> - if autovacuum_work_mem is set then it is used instead of
> maintenance_work_mem by autovacuum.
> - the autovacuum_work_mem guc has a "sighup" context and the global
> variable used in the code is correctly refreshed during a reload.

Right.

> - a 1MB lower limit for autovacuum_work_mem is enforced.

Right, just like maintenance_work_mem. The difference being that we
cannot enforce it with the same standard mechanism, because we still
need to make -1 values possible. This happens in our callback, in the
style of wal_buffers.

> build (platform is fedora 18):
> - patch (context format) applies to current HEAD with offsets (please rebase).

It's been rebased.

> questions/comments:
> - should the category of the guc be "RESOURCES_MEM" (as in the patch)
> or "AUTOVACUUM"? seems to fit in both, but it's really autovacuum
> specific.

Well, log_autovacuum_min_duration is also very autovacuum specific,
but is LOGGING_WHAT, so I've left autovacuum_work_mem RESOURCES_MEM.
It's not a fixed allocation of shared memory.

> - could you also add documentation to the autovacuum section of
> maintenance.sgml? I think people tuning their autovacuum are likely to
> look there for guidance.

I don't want to add a reference there as long as there is no
maintenance_work_mem reference. I think that perform.sgml is a more
likely candidate, though I haven't added anything there either, since
it's just talking about increasing maintenance_work_mem in a
controlled context.

> - could you update the comments at the top of vacuumlazy.c to reflect
> this new feature?

Seems reasonable.

Revision attached.
--
Peter Geoghegan