Date: Tue, 17 Mar 2020 10:24:09 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: Laurenz Albe <laurenz.albe@cybertec.at>
Cc: Andres Freund <andres@anarazel.de>,
	Masahiko Sawada <masahiko.sawada@2ndquadrant.com>,
	David Rowley <dgrowleyml@gmail.com>,
	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>,
	PostgreSQL Developers <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: <20200317152409.GW26184@telsasoft.com>
References: 
 <CAApHDvq=Gaycio_60j9bHs3CTgkeLkUCH5kbyHbVuYryQbDfpA@mail.gmail.com>
 <CA+fd4k6MhGk95y53k-tLRB9zz5fecwiJhTrma9p0b3=rDaEr7g@mail.gmail.com>
 <CAApHDvrACF7jB_hbq75XiaRtFRUq9BcXm=f15icu3+Q9xD+YQA@mail.gmail.com>
 <CA+fd4k5HwScjcUbjSuZXWyU6JNENsOebktdi1ft39rJqLZ9N2A@mail.gmail.com>
 <20200316124713.GH26184@telsasoft.com>
 <9c9b7d19d17ad5e4e0ee10b8426fa1b78dd24c0a.camel@cybertec.at>
 <20200316201315.dgkeedl6rp2wdjje@alap3.anarazel.de>
 <19be41199e7b303bbecc1066bd9f011d95882aa8.camel@cybertec.at>
 <20200316213459.kh6xln3z35hgjodd@alap3.anarazel.de>
 <7c132215f0e947af7d9ccb7a340ad6e81b369c24.camel@cybertec.at>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <7c132215f0e947af7d9ccb7a340ad6e81b369c24.camel@cybertec.at>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

On Tue, Mar 17, 2020 at 01:14:02AM +0100, Laurenz Albe wrote:
> lazy_check_needs_freeze() is only called for an aggressive vacuum, which
> this isn't.

> --- a/src/backend/access/heap/vacuumlazy.c
> +++ b/src/backend/access/heap/vacuumlazy.c
> @@ -1388,17 +1388,26 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
>  			else
>  			{
>  				bool		tuple_totally_frozen;
> +				bool		freeze_all;
>  
>  				num_tuples += 1;
>  				hastup = true;
>  
> +				/*
> +				 * If any tuple was already frozen in the block and this is
> +				 * an insert-only vacuum, we might as well freeze all other
> +				 * tuples in that block.
> +				 */
> +				freeze_all = params->is_insert_only && has_dead_tuples;
> +

You're checking if any (previously-scanned) tuple was *dead*, but I think you
need to check nfrozen>=0.

Also, this will fail to freeze tuples on a page which *could* be
oppotunistically-frozen, but *follow* the first tuple which *needs* to be
frozen.

I think Andres was thinking this would maybe be an optimization independent of
is_insert_only (?)

>  				/*
>  				 * Each non-removable tuple must be checked to see if it needs
>  				 * freezing.  Note we already have exclusive buffer lock.
>  				 */
>  				if (heap_prepare_freeze_tuple(tuple.t_data,
>  											  relfrozenxid, relminmxid,
> -											  FreezeLimit, MultiXactCutoff,
> +											  freeze_all ? 0 : FreezeLimit,
> +											  freeze_all ? 0 : MultiXactCutoff,
>  											  &frozen[nfrozen],
>  											  &tuple_totally_frozen))

> +	/* normal autovacuum shouldn't freeze aggressively */
> +	*insert_only = false;

Aggressively is a bad choice of words.  In the context of vacuum, it usually
means "visit all pages, even those which are allvisible".

-- 
Justin