Date: Thu, 19 Mar 2020 09:09:45 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: David Rowley <dgrowleyml@gmail.com>
Cc: Andres Freund <andres@anarazel.de>,
	Laurenz Albe <laurenz.albe@cybertec.at>,
	Tomas Vondra <tomas.vondra@2ndquadrant.com>,
	Amit Kapila <amit.kapila16@gmail.com>,
	Masahiko Sawada <sawada.mshk@gmail.com>,
	Darafei =?utf-8?Q?Kom=D1=8Fpa?= Praliaskouski <me@komzpa.net>,
	pgsql-hackers@lists.postgresql.org,
	Alvaro Herrera <alvherre@2ndquadrant.com>,
	Michael Banck <mbanck@gmx.net>
Subject: Re: Berserk Autovacuum (let's save next Mandrill)
Message-ID: <20200319140945.GS26184@telsasoft.com>
References: 
 <CAApHDvrUQazEd3F491ubZzU65B=e=O37bPcvXtPAH5vSn+9YUw@mail.gmail.com>
 <602873766faa0e9200a60dcc26dc10c636761d5d.camel@cybertec.at>
 <CAApHDvrdVpYQUqk5uTR-iVwnW=JbEwX86zngCj5v06Ug77MPVw@mail.gmail.com>
 <91c110aad4b8653d6433d42fd707bd16074c79a5.camel@cybertec.at>
 <CAApHDvqSBewvq-jmY8+gM=MgAnanfoCbg89nCYihtZ8WKSOi4g@mail.gmail.com>
 <CAApHDvoGabCHjT3uCV-kjxHFRQkX4hDjPzdp-vfAAdiE3teCbg@mail.gmail.com>
 <20200313184442.GS29065@telsasoft.com>
 <20200313213851.ejrk5gptnmp65uoo@alap3.anarazel.de>
 <20200319060658.GQ26184@telsasoft.com>
 <CAApHDvrMR64pGei+xFTcphqdrRouNZLunQs40f73hmU9un+9rg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CAApHDvrMR64pGei+xFTcphqdrRouNZLunQs40f73hmU9un+9rg@mail.gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

On Thu, Mar 19, 2020 at 09:52:11PM +1300, David Rowley wrote:
> On Thu, 19 Mar 2020 at 19:07, Justin Pryzby <pryzby@telsasoft.com> wrote:
> >
> > On Fri, Mar 13, 2020 at 02:38:51PM -0700, Andres Freund wrote:
> > > On 2020-03-13 13:44:42 -0500, Justin Pryzby wrote:
> > > > Having now played with the patch, I'll suggest that 10000000 is too high a
> > > > threshold.  If autovacuum runs without FREEZE, I don't see why it couldn't be
> > > > much lower (100000?) or use (0.2 * n_ins + 50) like the other autovacuum GUC.
> > >
> > > ISTM that the danger of regressing workloads due to suddenly repeatedly
> > > scanning huge indexes that previously were never / rarely scanned is
> > > significant (if there's a few dead tuples, otherwise most indexes will
> > > be able to skip the scan since the vacuum_cleanup_index_scale_factor
> > > introduction)).
> >
> > We could try to avoid that issue here:
> >
> > |        /* If any tuples need to be deleted, perform final vacuum cycle */
> > |        /* XXX put a threshold on min number of tuples here? */
> > |        if (dead_tuples->num_tuples > 0)
> > |        {
> > |                /* Work on all the indexes, and then the heap */
> > |                lazy_vacuum_all_indexes(onerel, Irel, indstats, vacrelstats,
> > |                                                                lps, nindexes);
> > |
> > |                /* Remove tuples from heap */
> > |                lazy_vacuum_heap(onerel, vacrelstats);
> > |        }
> >
> > As you said, an insert-only table can skip scanning indexes, but an
> > insert-mostly table currently cannot.
> >
> > Maybe we could skip the final index scan if we hit the autovacuum insert
> > threshold?
> >
> > I still don't like mixing the thresholds with the behavior they imply, but
> > maybe what's needed is better docs describing all of vacuum's roles and its
> > procedure and priority in executing them.
> >
> > The dead tuples would just be cleaned up during a future vacuum, right ?  So
> > that would be less efficient, but (no surprise) there's a balance to strike and
> > that can be tuned.  I think that wouldn't be an issue for most people; the
> > worst case would be if you set high maint_work_mem, and low insert threshold,
> > and you got increased bloat.  But faster vacuum if we avoided idx scans.
> >
> > That might allow more flexibility in our discussion around default values for
> > thresholds for insert-triggered vacuum.
> 
> We went over this a bit already. The risk is that if you have an
> insert-mostly table and always trigger an auto-vacuum for inserts and
> never due to dead tuples, then you'll forego the index cleanup every
> time causing the indexes to bloat over time.

At the time, we were talking about skipping index *cleanup* phase.
Which also incurs an index scan.
>+		tab->at_params.index_cleanup = insert_only ? VACOPT_TERNARY_DISABLED : VACOPT_TERNARY_DEFAULT;
We decided not to skip this, since it would allow index bloat, if vacuum were
only ever run due to inserts, and cleanup never happened.

I'm suggesting the possibility of skipping not index *cleanup* but index (and
heap) *vacuum*.  So that saves an index scan itself, and I think implies later
skipping cleanup (since no index tuples were removed).  But more importantly, I
think if we skip that during an insert-triggered vacuum, the dead heap tuples
are still there during the next vacuum instance.  So unlike index cleanup
(where we don't keep track of the total number of dead index tuples), this can
accumulate over time, and eventually trigger index+heap vacuum, and cleanup.

> I think any considerations to add some sort of threshold on dead
> tuples before cleaning the index should be considered independently.

+1, yes.  I'm hoping to anticipate and mitigate any objections and regressions
more than raise them.

-- 
Justin