From: | Peter Geoghegan <pg(at)heroku(dot)com> |
---|---|
To: | Nigel Heron <nheron(at)querymetrics(dot)com> |
Cc: | Magnus Hagander <magnus(at)hagander(dot)net>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: review: autovacuum_work_mem |
Date: | 2013-11-19 04:36:01 |
Message-ID: | CAM3SWZS8=ZPjYODSQBxEjCHUj74W+vqkBFwjPotVM=YAd72E4A@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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(at)querymetrics(dot)com> wrote:
> 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.
> 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 | Content-Type | Size |
---|---|---|
autovacuum_work_mem.v2.2013_11_18.patch | text/x-patch | 8.1 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Amit Kapila | 2013-11-19 05:02:46 | Re: Heavily modified big table bloat even in auto vacuum is running |
Previous Message | Amit Kapila | 2013-11-19 04:28:34 | Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: Proposal for Allow postgresql.conf values to be changed via SQL [review]) |