On Thu, Jun 17, 2021 at 2:14 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> Thank you for updating the patch! Here are comments on v2 patch:
Thanks for the review!
Attached is v3, which has all the changes that you suggested (plus the
doc stuff from Justin).
I also renamed the "default" VacOptTernaryValue (actually now called
VacOptValue) value -- it seems clearer to call this "unspecified".
Because it represents a state that has nothing to do with the default
of the reloption or GUC. Really, it means "VACUUM command didn't have
this specified explicitly" (note that this means that it always starts
out "default" in an autovacuum worker). Unspecified seems much clearer
because it directly expresses "fall back on the reloption, and then
fall back on the reloption's default". I find this much clearer -- it
is unspecified, but will have to *become* specified later, so that
vacuumlazy.c has a truly usable value ("unspecified" is never usable
in vacuumlazy.c).
> VacOptTernaryValue is no longer a ternary value. Can we rename it
> something like VacOptValue?
As I said, done that way.
> We should specify TRUE instead.
Ooops. Fixed.
> --force-index-cleanup option isn't shown in the help message.
Fixed.
> ---
> I think we also improve the tab completion for INDEX_CLEANUP option.
Fixed.
> How about listing the available values of INDEX_CLEANUP here instead
> of enum? For example, we do a similar thing in the description of
> FORMAT option of EXPLAIN command. It would be easier to perceive all
> available values.
That looks much better. Fixed.
> It should be --force-index-cleanup.
Fixed.
--
Peter Geoghegan