Re: review: autovacuum_work_mem

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

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:
</messages/by-id/CAM3SWZTwLA8Ef2DTvbwM1b1zEVU_eN3N4rReGNU5_zFyjNGi6w@mail.gmail.com&gt;

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
</messages/by-id/CAM3SWZTwLA8Ef2DTvbwM1b1zEVU_eN3N4rReGNU5_zFyjNGi6w@mail.gmail.com&gt;

* 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
</messages/by-id/CAM3SWZTwLA8Ef2DTvbwM1b1zEVU_eN3N4rReGNU5_zFyjNGi6w@mail.gmail.com&gt;

* 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.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers