Re: Berserk Autovacuum (let's save next Mandrill) - Mailing list pgsql-hackers

On Tue, 10 Mar 2020 at 09:56, David Rowley <dgrowleyml@gmail.com> wrote:
>
> On Sat, 7 Mar 2020 at 03:45, Laurenz Albe <laurenz.albe@cybertec.at> wrote:
> > Updated patch attached.
>
> Great. I'll have a look.

I don't really have many complaints about the v4 patch.  However,
during my pass of it, I did note down a few things that you might want
to have a look at.

1. Do we need to change documentation on freeze_min_age to mention
that it does not apply in all cases? I'm leaning towards not changing
this as `VACUUM FREEZE` is also an exception to this, which I don't
see mentioned.

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.

See heap_vacuum_rel()

/*
* 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.
*/
aggressive = TransactionIdPrecedesOrEquals(onerel->rd_rel->relfrozenxid,
   xidFullScanLimit);
aggressive |= MultiXactIdPrecedesOrEquals(onerel->rd_rel->relminmxid,
  mxactFullScanLimit);
if (params->options & VACOPT_DISABLE_PAGE_SKIPPING)
aggressive = true;

3. The following DEBUG3 elog should be updated to include the new values:

elog(DEBUG3, "%s: vac: %.0f (threshold %.0f), anl: %.0f (threshold %.0f)",
NameStr(classForm->relname),
vactuples, vacthresh, anltuples, anlthresh);

Someone might be confused at why auto-vacuum is running if you don't
put those in.

4. This would be nicer if you swapped the order of the operands to the
< condition and replaced the operator with >. That'll match the way it
is done above.

/*
* If the number of inserted tuples exceeds the threshold and no
* vacuum is necessary for other reasons, run an "insert-only" vacuum
* that freezes aggressively.
*/
if (!(*dovacuum) && vacinsthresh < tabentry->inserts_since_vacuum)
{
*dovacuum = true;
*freeze_all = true;
}

It would also be nicer if you assigned the value of
tabentry->inserts_since_vacuum to a variable, so as to match what the
other code there is doing. That'll also make the change for #3 neater.

5. The following text:

    A threshold similar to the above is calculated from
    <xref linkend="guc-autovacuum-vacuum-insert-threshold"/> and
    <xref linkend="guc-autovacuum-vacuum-insert-scale-factor"/>.
    Tables that have received more inserts than the calculated threshold
    since they were last vacuumed (and are not eligible for vacuuming for
    other reasons) will be vacuumed to reduce the impact of a future
    anti-wraparound vacuum run.

I think "... will be vacuumed with the equivalent of having <xref
linkend="guc-vacuum-freeze-min-age"/> set to <literal>0</literal>".
I'm not sure we need to mention the reduction of impact to
anti-wraparound vacuums.

6. Please run the regression tests and make sure they pass. The
"rules" test is currently failing due to the new column in
"pg_stat_all_tables"

Apart from the above, does anyone else have objections or concerns
with the patch?  I'd like to take a serious look at pushing it once
the above points are resolved.