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.

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

On Mon, Nov 18, 2013 at 11:36 PM, Peter Geoghegan <pg(at)heroku(dot)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.