Re: New vacuum option to do only freezing

On Wed, Apr 3, 2019 at 10:56 AM Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>
> Hello.
>
> At Mon, 1 Apr 2019 14:26:15 +0900, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote in <CAD21AoCKKwvgQgWxKwPPmVFJjQVj6c=oV9dtdiRthdV+WjnD4w(at)mail(dot)gmail(dot)com>
> > On Sat, Mar 30, 2019 at 5:04 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > >
> > > On Fri, Mar 29, 2019 at 12:27 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> > > > Yeah, but since multiple relations might be specified in VACUUM
> > > > command we need to process index_cleanup option after opened each
> > > > relations. Maybe we need to process all options except for
> > > > INDEX_CLEANUP in ExecVacuum() and pass VacuumStmt down to vacuum_rel()
> > > > and process it in manner of you suggested after opened the relation.
> > > > Is that right?
> > >
> > > Blech, no, let's not do that. We'd better use some method that can
> > > indicate yes/no/default. Something like psql's trivalue thingy, but
> > > probably not exactly that. We can define an enum for this purpose,
> > > for example - VACUUM_INDEX_CLEANUP_{ENABLED,DISABLED,DEFAULT}. Or
> > > maybe there's some other way. But let's not pass bits of the parse
> > > tree around any more than really required.
> >
> > I've defined an enum VacOptTernaryValue representing
> > enabled/disabled/default and added index_cleanup variable as the new
>

Thank you for reviewing the patch!

> It is defined as ENABLED=0, DISABLED=1, DEFAULT=2. At leat
> ENABLED=0 and DISABLED=1 are misleading.

Indeed, will fix.

>
> > enum type to VacuumParams. The vacuum options that uses the reloption
> > value as the default value such as index cleanup and new truncation
> > option can use this enum and set either enabled or disabled after
> > opened the relation when it’s set to default. Please find the attached
> > patches.
>
> +static VacOptTernaryValue get_vacopt_ternary_value(DefElem *def);
>
> This is actually a type converter of boolean. It is used to read
> VACUUM option but not used to read reloption. It seems useless.
>
>
> Finally the ternary value is determined to true or false before
> use. It is simple that index_cleanup finaly be read as bool. We
> could add another boolean to indicate that the value is set or
> not, but I think it would be better that the ternary type is a
> straightfoward expansion of bool.{DEFAULT = -1, DISABLED = 0,
> ENABLED = 1} and make sure that index_cleanup is not DEFAULT at a
> certain point.
>
> So, how about this?
>
> #define VACOPT_TERNARY_DEFAULT -1
> typedef int VacOptTernaryValue; /* -1 is undecided, 0 is false, 1 is true */

Hmm, if we do that we set either VAOPT_TERNARY_DEFAULT, true or false
to index_cleanup, but I'm not sure this is a good approach. I think we
would want VACOPT_TERNARY_TRUE and VACOPT_TERNARY_FALSE as we defined
new type as a ternary value and already have VACOPT_TERNARY_DEFAULT.

>
> /* No longer the value mustn't be left DEFAULT */
> Assert (params->index_cleanup != VACOPT_TERNARY_DEFAULT);

Agreed, will add it.

>
>
> > > > > I think it would be better to just ignore the INDEX_CLEANUP option
> > > > > when FULL is specified.
> > > >
> > > > Okay, but why do we ignore that in this case while we complain in the
> > > > case of FULL and DISABLE_PAGE_SKIPPING?
> > >
> > > Well, that's a fair point, I guess. If we go that that route, we'll
> > > need to make sure that setting the reloption doesn't prevent VACUUM
> > > FULL from working -- the complaint must only affect an explicit option
> > > on the VACUUM command line. I think we should have a regression test
> > > for that.
> >
> > I've added regression tests. Since we check it before setting
> > index_cleanup based on reloptions we doesn't prevent VAUCUM FULL from
> > working even when the vacuum_index_cleanup is false.
>
> + errmsg("VACUUM option INDEX_CLEANUP cannot be set to false with FULL")));
>
> I'm not one to talk on this, but this seems somewhat confused.
>
> "VACUUM option INDEX_CLEANUP cannot be set to false with FULL being specified"
>
> or
>
> "INDEX_CLEANUP cannot be disabled for VACUUM FULL"?

I prefer the former, will fix.

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

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

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center