Re: VACUUM (DISABLE_PAGE_SKIPPING on) - Mailing list pgsql-hackers

On Tue, Dec 1, 2020 at 10:45 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Fri, Nov 20, 2020 at 8:47 PM Simon Riggs <simon@2ndquadrant.com> wrote:
> >
> > On Fri, 20 Nov 2020 at 10:15, Simon Riggs <simon@2ndquadrant.com> wrote:
> > >
> > > On Fri, 20 Nov 2020 at 01:40, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > > >
> > > > On Thu, Nov 19, 2020 at 8:02 PM Simon Riggs <simon@2ndquadrant.com> wrote:
> > > > >
> > > > > On Wed, 18 Nov 2020 at 17:59, Robert Haas <robertmhaas@gmail.com> wrote:
> > > > > >
> > > > > > On Wed, Nov 18, 2020 at 12:54 PM Simon Riggs <simon@2ndquadrant.com> wrote:
> > > > > > > Patches attached.
> > > > > > > 1. vacuum_anti_wraparound.v2.patch
> > > > > > > 2. vacuumdb_anti_wrap.v1.patch - depends upon (1)
> > > > > >
> > > > > > I don't like the use of ANTI_WRAPAROUND as a name for this new option.
> > > > > > Wouldn't it make more sense to call it AGGRESSIVE? Or maybe something
> > > > > > else, but I dislike anti-wraparound.
> > > > >
> > > > > -1 for using the term AGGRESSIVE, which seems likely to offend people.
> > > > > I'm sure a more descriptive term exists.
> > > >
> > > > Since we use the term aggressive scan in the docs, I personally don't
> > > > feel unnatural about that. But since this option also disables index
> > > > cleanup when not enabled explicitly, I’m concerned a bit if user might
> > > > get confused. I came up with some names like FEEZE_FAST and
> > > > FREEZE_MINIMAL but I'm not sure these are better.
> > >
> > > FREEZE_FAST seems good.
> > >
> > > > BTW if this option also disables index cleanup for faster freezing,
> > > > why don't we disable heap truncation as well?
> > >
> > > Good idea
> >
> > Patch attached, using the name "FAST_FREEZE" instead.
> >
>
> Thank you for updating the patch.
>
> Here are some comments on the patch.
>
> ----
> -   if (params->options & VACOPT_DISABLE_PAGE_SKIPPING)
> +   if (params->options & VACOPT_DISABLE_PAGE_SKIPPING ||
> +       params->options & VACOPT_FAST_FREEZE)
>
> I think we need to update the following comment that is above this
> change as well:
>
>     /*
>      * We request an aggressive scan if the table's frozen Xid is now older
>      * than or equal to the requested Xid full-table scan limit; or if the
>      * table's minimum MultiXactId is older than or equal to the requested
>      * mxid full-table scan limit; or if DISABLE_PAGE_SKIPPING was specified.
>      */
>
> This mentions only DISABLE_PAGE_SKIPPING now. Or the second idea is to
> set both params.freeze_table_age and params.multixact_freeze_table_age
> to 0 at ExecVacuum() instead of getting aggressive turned on here.
> Considering the consistency between FREEZE and FREEZE_FAST, we might
> want to take the second option.
>
> ---
> +   if (fast_freeze &&
> +       params.index_cleanup == VACOPT_TERNARY_DEFAULT)
> +       params.index_cleanup = VACOPT_TERNARY_DISABLED;
> +
> +   if (fast_freeze &&
> +       params.truncate == VACOPT_TERNARY_DEFAULT)
> +       params.truncate = VACOPT_TERNARY_DISABLED;
> +
> +   if (fast_freeze && freeze)
> +       ereport(ERROR,
> +               (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> +                errmsg("cannot specify both FREEZE and FAST_FREEZE
> options on VACUUM")));
> +
>
> I guess that you disallow enabling both FREEZE and FAST_FREEZE because
> it's contradictory, which makes sense to me. But it seems to me that
> enabling FAST_FREEZE, INDEX_CLEANUP, and TRUNCATE is also
> contradictory because it will no longer be “fast”. The purpose of this
> option to advance relfrozenxid as fast as possible by disabling index
> cleanup, heap truncation etc. Is there any use case where a user wants
> to enable these options (FAST_FREEZE, INDEX_CLEANUP, and TRUNCATE) at
> the same time? If not, probably it’s better to either disallow it or
> have FAST_FREEZE overwrites these two settings even if the user
> specifies them explicitly.
>

I sent some review comments a month ago but the patch was marked as
"needs review”, which was incorrect So I think "waiting on author" is
a more appropriate state for this patch. I'm switching the patch as
"waiting on author".

Regards,

--
Masahiko Sawada
EnterpriseDB:  https://www.enterprisedb.com/