Hi Simon,
On Mon, Jan 4, 2021 at 11:45 PM Masahiko Sawada <sawada.m...@gmail.com> wrote:
>
> On Tue, Dec 1, 2020 at 10:45 AM Masahiko Sawada <sawada.m...@gmail.com> wrote:
> >
> > On Fri, Nov 20, 2020 at 8:47 PM Simon Riggs <si...@2ndquadrant.com> wrote:
> > >
> > > On Fri, 20 Nov 2020 at 10:15, Simon Riggs <si...@2ndquadrant.com> wrote:
> > > >
> > > > On Fri, 20 Nov 2020 at 01:40, Masahiko Sawada <sawada.m...@gmail.com>
> > > > wrote:
> > > > >
> > > > > On Thu, Nov 19, 2020 at 8:02 PM Simon Riggs <si...@2ndquadrant.com>
> > > > > wrote:
> > > > > >
> > > > > > On Wed, 18 Nov 2020 at 17:59, Robert Haas <robertmh...@gmail.com>
> > > > > > wrote:
> > > > > > >
> > > > > > > On Wed, Nov 18, 2020 at 12:54 PM Simon Riggs
> > > > > > > <si...@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".
>
Status update for a commitfest entry.
This entry has been "Waiting on Author" status and the patch has not
been updated since Nov 30. Are you still planning to work on this?
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
