On Sun, Oct 20, 2013 at 7:21 AM, Magnus Hagander <mag...@hagander.net> wrote: > On Sun, Oct 20, 2013 at 2:22 AM, Peter Geoghegan <p...@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. > >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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
