Re: review: autovacuum_work_mem

On Sun, Oct 20, 2013 at 7:21 AM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> On Sun, Oct 20, 2013 at 2:22 AM, Peter Geoghegan <pg(at)heroku(dot)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.
>

>But I'd suggest just a:
>int vac_work_mem = (IsAutoVacuumWorkerProcess() && autovacuum_work_mem
>!= -1) ? autovacuum_work_mem : maintenance_work_mem;
>
>and not sending around a boolean flag through a bunch of places when
>it really means just the same thing,

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

>
> 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. And -1 doesn't actually disable
> it. I suggest following the pattern of the other parameters that work
> the same way, which would then just be:
>
> #autovacuum_work_mem = -1 # amount of memory for each autovacuum
> worker. -1 means use maintenance_work_mem
>

+1

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.
- a 1MB lower limit for autovacuum_work_mem is enforced.

build (platform is fedora 18):
- patch (context format) applies to current HEAD with offsets (please rebase).
- documentation patches included.
- "make" doesn't produce any extra warnings.
- "make check" passes all tests (no extra regression tests).

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.
- 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.
- could you update the comments at the top of vacuumlazy.c to reflect
this new feature?

-nigel.