On Tue, 2020-03-10 at 13:53 +1300, David Rowley wrote:
> 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.
I agree with that. Too little documentation is bad, but too much of
it can also confuse and make it hard to find the needle in the haystack.
> 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.
Agreed, see below.
> 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);
Done.
> 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.
Changed that way.
> 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.
Done like that.
I left in the explanation of the purpose of this setting.
Understanding the purpose of the GUCs will make it easier to tune them
correctly.
> 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"
Oops, sorry. I ran pgindent, but forgot to re-run the regression tests.
Done.
Attached is V5, which also fixes the bug discovered my Masahiko Sawada.
He made an interesting suggestion which we should consider before committing.
Yours,
Laurenz Albe
