Re: New vacuum option to do only freezing - Mailing list pgsql-hackers

Hello.

At Mon, 1 Apr 2019 14:26:15 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in
<CAD21AoCKKwvgQgWxKwPPmVFJjQVj6c=oV9dtdiRthdV+WjnD4w@mail.gmail.com>
> On Sat, Mar 30, 2019 at 5:04 AM Robert Haas <robertmhaas@gmail.com> wrote:
> >
> > On Fri, Mar 29, 2019 at 12:27 PM Masahiko Sawada <sawada.mshk@gmail.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

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

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

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


> > > > 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"?


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, and maybe we
should complain in WARNING or NOTICE that the relopt is ignored.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center