Thread: Re: review: autovacuum_work_mem
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. > >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.
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
Attachment
On Mon, Nov 18, 2013 at 11:36 PM, Peter Geoghegan <pg@heroku.com> wrote: > 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). > sorry about that, when i added "review" to the subject gmail removed the thread info. for reference the original thread started here: <http://www.postgresql.org/message-id/CAM3SWZTwLA8Ef2DTvbwM1b1zEVU_eN3N4rReGNU5_zFyjNGi6w@mail.gmail.com> > > Revision attached. > -- > Peter Geoghegan Review for Peter Geoghegan's v2 patch in CF 2013-11: https://commitfest.postgresql.org/action/patch_view?id=1262 Submission review ----------------- * Is the patch in a patch format which has context? Yes * Does it apply cleanly to the current git master (04eee1fa9ee80dabf7cf4b8b9106897272e9b291)? patching file src/backend/commands/vacuumlazy.c Hunk #2 succeeded at 1582 (offset 1 line). * Does it include reasonable tests, necessary doc patches, etc? Documentation patches included. No additional tests. Usability review ----------------- * Does the patch actually implement that? Yes. * Do we want that? Yes. The original thread has references, see <http://www.postgresql.org/message-id/CAM3SWZTwLA8Ef2DTvbwM1b1zEVU_eN3N4rReGNU5_zFyjNGi6w@mail.gmail.com> * Do we already have it? No. * Does it follow SQL spec, or the community-agreed behavior? SQL spec: n/a community: Yes. The original thread has references, see <http://www.postgresql.org/message-id/CAM3SWZTwLA8Ef2DTvbwM1b1zEVU_eN3N4rReGNU5_zFyjNGi6w@mail.gmail.com> * Does it include pg_dump support (if applicable)? n/a Feature test ----------------- * Does the feature work as advertised? Yes. * Are there corner cases the author has failed to consider? None that i can see. * Are there any assertion failures or crashes? No. Performance review ----------------- * Does the patch slow down simple tests? No. * If it claims to improve performance, does it? n/a * Does it slow down other things? No. Coding review ----------------- * Does it follow the project coding guidelines? Yes. * Are there portability issues? None that i can see. * Will it work on Windows/BSD etc? None that i can see. (I only tested it on linux though) * Does it do what it says, correctly? Yes. * Does it produce compiler warnings? No. * Can you make it crash? No. Architecture review ----------------- * Is everything done in a way that fits together coherently with other features/modules? Yes. * Are there interdependencies that can cause problems? No. -nigel.