Re: VACUUM (DISABLE_PAGE_SKIPPING on)

On Fri, Nov 20, 2020 at 8:47 PM Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>
> On Fri, 20 Nov 2020 at 10:15, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> >
> > On Fri, 20 Nov 2020 at 01:40, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> > >
> > > On Thu, Nov 19, 2020 at 8:02 PM Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> > > >
> > > > On Wed, 18 Nov 2020 at 17:59, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > > > >
> > > > > On Wed, Nov 18, 2020 at 12:54 PM Simon Riggs <simon(at)2ndquadrant(dot)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.

Regards,

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