On Wed, Apr 3, 2019 at 1:17 PM Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:
>
> At Wed, 3 Apr 2019 12:10:03 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in
<CAD21AoBkaHka5sav5N6vvoKS9qpmrWRBdyNGP8S7M0SsPd0iyQ@mail.gmail.com>
> > > And in the following part:
> > >
> > > + /* Set index cleanup option based on reloptions */
> > > + if (params->index_cleanup == VACUUM_OPTION_DEFAULT)
> > > + {
> > > + if (onerel->rd_options == NULL ||
> > > + ((StdRdOptions *) onerel->rd_options)->vacuum_index_cleanup)
> > > + params->index_cleanup = VACUUM_OPTION_ENABLED;
> > > + else
> > > + params->index_cleanup = VACUUM_OPTION_DISABLED;
> > > + }
> > > +
> > >
> > > The option should not be false while VACUUM FULL,
> >
> > I think that we need to complain only when INDEX_CLEANUP option is
> > disabled by an explicit option on the VACUUM command and FULL option
> > is specified. It's no problem when vacuum_index_cleanup is false and
> > FULL option is true. Since internally we don't use index cleanup when
> > vacuum full I guess that we don't need to require index_cleanup being
> > always true even when full option is specified.
>
> I know it's safe. It's just about integrity of option values. So
> I don't insist on that.
>
> > > and maybe we
> > > should complain in WARNING or NOTICE that the relopt is ignored.
> >
> > I think when users want to control index cleanup behavior manually
> > they specify INDEX_CLEANUP option on the VACUUM command. So it seems
> > to me that overwriting a reloption by an explicit option would be a
> > natural behavior. I'm concerned that these message would rather
> > confuse users.
>
> If it "cannot be specified with FULL", it seems strange that it's
> safe being specified by reloptions.
>
> I'm rather thinking that INDEX_CLEANUP = false is ignorable even
> being specified with FULL option, aand DISABLE_PAGE_SKIPPING for
> VACUUM FULL shuld be ignored since VACUUM FULL doesn't skip pages
> in the first place.
>
> Couldn't we silence the DISABLE_PAGE_SKIPPING & FULL case instead
> of complaining about INDEX_CLEANUP & FULL? If so, I feel just
> ignoring the relopt cases is reasonable.
Agreed with being silent even when INDEX_CLEANUP/vacuum_index_cleanup
= false and FULL = true. For DISABLE_PAGE_SKIPPING, it should be a
separate patch and changes the existing behavior. Maybe need other
discussion.
For VacOptTernaryValue part, I've incorporated the review comments but
left the new enum type since it seems to be more straightforward for
now. I might change that if there are other way.
Attached the updated version patches including the
DISABLE_PAGE_SKIPPING part (0003).
Regards,
--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center