Date: Fri, 13 Mar 2020 13:44:42 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: David Rowley <dgrowleyml@gmail.com>
Cc: 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>,
	Andres Freund <andres@anarazel.de>,
	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: <20200313184442.GS29065@telsasoft.com>
References: <20200107180524.mqz456t2pmr7crpw@development>
 <98a1422dece118877ebbdc20cb78209f6d12866e.camel@cybertec.at>
 <0cd8685047ac86fdf9c5446a24ff3aed8755e290.camel@cybertec.at>
 <2207744f6cc70813d9000510b6bf03ce85db6675.camel@cybertec.at>
 <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>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CAApHDvoGabCHjT3uCV-kjxHFRQkX4hDjPzdp-vfAAdiE3teCbg@mail.gmail.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Precedence: bulk

On Tue, Mar 10, 2020 at 01:53:42PM +1300, David Rowley wrote:
> 2. Perhaps the documentation in maintenance.sgml should mention that
> the table will be vacuumed with the equivalent of having
> vacuum_freeze_min_age = 0, instead of:
> 
> "Such a vacuum will aggressively freeze tuples."
> 
> aggressive is the wrong word here. We call it an aggressive vacuum if
> we disable page skipping, not for setting the vacuum_freeze_min_age to
> 0.

Possible it would be better to run VACUUM *without* freeze_min_age=0 ?  (I get
confused and have to spend 20min re-reading the vacuum GUC docs every time I
deal with this stuff, so maybe I'm off).

As I understand, the initial motivation of this patch was to avoid disruptive
anti-wraparound vacuums on insert-only table.  But if vacuum were triggered at
all, it would freeze the oldest tuples, which is all that's needed; especially
since fd31cd2651 "Don't vacuum all-frozen pages.", those pages would never need
to be vacuumed again.  Recently written tuples wouldn't be frozen, which is ok,
they're handled next time.

Another motivation of the patch is to allow indexonly scan, for which the
planner looks at pages' "relallvisible" fraction (and at execution if a page
isn't allvisible, visits the heap).  Again, that happens if vacuum were run at
all.  Again, some pages won't be marked allvisible, which is fine, they're
handled next time.

I think freeze_min_age=0 could negatively affect people who have insert-mostly
tables (I'm not concerned, but that includes us).  If they consistently hit the
autovacuum insert threshold before the cleanup threshold for updated/deleted
tuples, any updated/deleted tuples would be frozen, which would be
wasteful:  

|One disadvantage of decreasing vacuum_freeze_min_age is that it might cause
|VACUUM to do useless work: freezing a row version is a waste of time if the row
|is modified soon thereafter (causing it to acquire a new XID). So the setting
|should be large enough that rows are not frozen until they are unlikely to
|change any more.

So my question is if autovacuum triggered by insert threshold should trigger
VACUUM with the same settings as a vacuum due to deleted tuples.  I realize the
DBA could just configure the thresholds so they'd hit vacuum for cleaning dead
tuples, so my suggestion maybe just improves the case with the default
settings.  It's possible to set the reloption autovacuum_freeze_min_age, which
I think supports the idea of running a vacuum normally and letting it (and the
DBA) decide what do with existing logic.

Also, there was a discussion about index cleanup with the conclusion that it
was safer not to skip it, since otherwise indexes might bloat.  I think that's
right, since vacuum for cleanup is triggered by the number of dead heap tuples.
To skip index cleanup, I think you'd want a metric for
n_dead_since_index_cleanup.  (Or maybe analyze could track dead index tuples
and trigger vacuum of each index separately).

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.  

-- 
Justin