On Tue, 2020-03-17 at 10:24 -0500, Justin Pryzby wrote:
> > --- 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.
Yes, that was a silly typo.
> 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 am aware of that. I was trying to see if that went in the direction that
Andres intends before trying more invasive modifications.
> I think Andres was thinking this would maybe be an optimization independent of
> is_insert_only (?)
I wasn't sure.
In the light of that, I have ripped out that code again.
Also, since aggressive^H^H^H^H^H^H^H^H^H^Hproactive freezing seems to be a
performance problem in some cases (pages with UPDATEs and DELETEs in otherwise
INSERT-mostly tables), I have done away with the whole freezing thing,
which made the whole patch much smaller and simpler.
Now all that is introduced are the threshold and scale factor and
the new statistics counter to track the number of inserts since the last
VACUUM.
> > + /* 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".
This is gone in the latest patch.
Updated patch attached.
Perhaps we can reach a consensus on this reduced functionality.
Yours,
Laurenz Albe
