On 07.04.23 02:52, David Rowley wrote:
On Fri, 7 Apr 2023 at 09:44, Melanie Plageman <melanieplage...@gmail.com> wrote:
Otherwise, LGTM.
Thanks for looking. I've also taken Justin's comments about the
README into account and fixed that part.
I've pushed the patch after a little more adjusting. I added some
text to the docs that mention larger VACUUM_BUFFER_LIMITs can speed up
vacuum and also a reason why they might not want to go nuts with it.
I came across these new options and had a little bit of trouble figuring
them out from the documentation. Maybe this could be polished a bit.
vacuumdb --help says
--buffer-usage-limit=BUFSIZE
I can guess what a "SIZE" might be, but is "BUFSIZE" different from a
"SIZE"? Maybe simplify here.
On the vacuumdb man page, the placeholder is
<replaceable class="parameter">buffer_usage_limit</replaceable>
which is yet another way of phrasing it. Maybe also use "size" here?
The VACUUM man page says
BUFFER_USAGE_LIMIT [ <replaceable ...>string</replaceable> ]
which had me really confused. The detailed description later doesn't
give any further explanation of possible values, except that
<literal>0</literal> is apparently a possible value, which in my mind is
not a string. Then there is a link to guc-vacuum-buffer-usage-limit,
which lifts the mystery that this is really just an integer setting with
possible memory-size units, but it was really hard to figure that out
from the start!
Moreover, on the VACUUM man page, right below BUFFER_USAGE_LIMIT, it
explains the different kinds of accepted values, and "string" wasn't
added there. Maybe also change this to "size" here and add an
explanation there what kinds of sizes are possible.
Finally, the locations of the new options in the various documentation
places seems a bit random. The vacuumdb --help output and the man page
appear to be mostly alphabetical, so --buffer-usage-limit should be
after -a/--all. (Also note that right now the option isn't even in the
same place in the --help output versus the man page.)
The order of the options on the VACUUM man page doesn't make any sense
anymore. This isn't really the fault of this patch, but maybe it's time
to do a fresh reordering there.
