Re: Decoupling antiwraparound autovacuum from special rules around auto cancellation

On Fri, Jan 13, 2023 at 2:00 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
> On 2023-01-12 16:08:06 -0500, Robert Haas wrote:
> > Normally, the XID age of a table never reaches autovacuum_freeze_max_age in
> > the first place.
>
> That's not at all my experience. I often see it being the primary reason for
> autovacuum vacuuming large tables on busy OLTP systems. Even without any
> longrunning transactions or such, with available autovac workers and without
> earlier autovacuums getting interrupted by locks. Once a table is large,
> reasonable scale factors require a lot of changes to accumulate to trigger an
> autovacuum, and during a vacuum a lot of transactions complete, leading to
> large tables having a significant age by the time autovac finishes.

I've definitely seen this. I've also noticed that TPC-C's stock and
customer tables (both of which are subject to many HOT updates) only
ever receive antiwraparound autovacuums, even with my very aggressive
autovacuum settings.

Overall, I think that it's quite common among the largest tables, even
when things are running normally.

> The most common "bad" reason for reaching autovacuum_freeze_max_age that I see
> is cost limits not allowing vacuum to complete on time.

There are many problems with the statistics driving this whole
process, that I won't rehash right now. I actually think that the
whole idea of relying on statistical sampling for dead tuples is
fundamentally just bogus (that's not how statistics work in general),
though I have quite a few less fundamental and more concrete
complaints about the statistics just being wrong on their own terms.

> Perhaps we should track how often autovacuum was triggered by what reason in
> a relation's pgstats? That'd make it a lot easier to collect data, both for
> tuning the thresholds on a system, and for hacking on postgres.

That would definitely be useful.

> Tracking the number of times autovacuum was interruped due to a lock request
> might be a good idea as well?

Also useful information worth having.

> I think it'd be a good idea to split off the part of the patch that introduces
> AutoVacType / adds logging for what triggered. That's independently useful,
> likely uncontroversial and makes the remaining patch smaller.

I like that idea.

Attached revision v4 breaks things up mechanically, along those lines
(no real changes to the code inself, though). The controversial parts
of the patch are indeed a fairly small proportion of the total
changes.

> I'd also add the trigger to the pg_stat_activity entry for the autovac
> worker. Additionally I think we should add information about using failsafe
> mode to the p_s_a entry.

I agree that that's all useful, but it seems like it can be treated as
later work.

> I've wished for a non-wraparound, xid age based, "autovacuum trigger" many
> times, FWIW. And I've seen plenty of places write their own userspace version
> of it, because without it they run into trouble. However, I don't like that
> the patch infers the various thresholds using magic constants / multipliers.

As I said, these details are totally negotiable, and likely could be a
lot better without much effort.

What are your concerns about the thresholds? For example, is it that
you can't configure the behavior directly at all? Something else?

> autovacuum_freeze_max_age is really a fairly random collection of things:
> 1) triggers autovacuum on tables based on age, in addition to the dead tuple /
> inserted tuples triggers
> 2) prevents auto-cancellation of autovacuum
> 3) starts autovacuum, even if autovacuum is disabled
>
> IME hitting 1) isn't a reason for concern, it's perfectly normal. Needing 2)
> to make progress is a bit more concerning. 3) should rarely be needed, but is
> a good safety mechanism.
>
> I doubt that controlling all of them via one GUC is sensible.

I agree, of course, but just to be clear: I don't think it matters
that we couple together 1 and 3. In fact it's good that we do that,
because the point to the user is that they cannot disable table-age
(i.e. what we current call antiwraparound) autovacuums -- that just
makes sense.

The only problem that I see is that item 2 is tied to the other items
from your list.

> If I understand the patch correctly, we now have the following age based
> thresholds for av:
>
> - force-enable autovacuum:
> oldest_datfrozenxid + autovacuum_freeze_max_age < nextXid
> - autovacuum based on age:
> freeze_max_age = Min(autovacuum_freeze_max_age, table_freeze_max_age)
> tableagevac = relfrozenxid < recentXid - freeze_max_age
> - prevent auto-cancellation:
> freeze_max_age = Min(autovacuum_freeze_max_age, table_freeze_max_age)
> prevent_auto_cancel_age = Min(freeze_max_age * 2, 1 billion)
> prevent_auto_cancel = reflrozenxid < recentXid - prevent_auto_cancel_age
>
> Is that right?

That summary looks accurate, but I'm a bit confused about why you're
asking the question this way. I thought that it was obvious that the
patch doesn't change most of these things.

The only mechanism that the patch changes is related to "prevent
auto-cancellation" behaviors -- which is now what the term
"antiwraparound" refers to. It does change the name of "autovacuum
based on age", though -- the name is now "table age autovacuum" (the
old name was antiwraparound autovacuum, of course). As I pointed out
to you already, it's mechanically impossible for any autovacuum to be
antiwraparound unless it's an XID table age/MXID table age autovacuum.

The naming convention I propose here makes it a little confusing for
us to discuss, but it seems like the best thing for users. Users'
basic intuitions about antiwraparound autovacuums (that they're scary
things needed because wraparound is starting to become a real concern)
don't need to change. If anything they become more accurate, because
antiwraparound autovacuums become non-routine -- which is really how
it should have been when autovacuum was first added IMV. Users have
rather good reasons to find antiwraparound autovacuums scary, even
though that's kind of wrong (it's really our fault for making it so
confusing for them, not their fault for being wrong).

> One thing I just noticed: Isn't it completely bonkers that we compute
> recentXid/recentMulti once at the start of a worker in
> relation_needs_vacanalyze()? That's fine for the calls in do_autovacuum()'s
> initial loops over all tables. But seems completely wrong for the later calls
> via table_recheck_autovac() -> recheck_relation_needs_vacanalyze() ->
> relation_needs_vacanalyze()?
>
> These variables really shouldn't be globals. It makes sense to cache them
> locally in do_autovacuum(), but reusing them
> recheck_relation_needs_vacanalyze() and sharing it between launcher and worker
> is bad.

I am not sure. I do hope that there isn't some subtle way in which the
design relies on that. It seems obviously weird, and so I have to
wonder if there is a reason behind it that isn't immediately apparent.

--
Peter Geoghegan