Re: Eagerly scan all-visible pages to amortize aggressive vacuum

Thanks for the review!

On Mon, Feb 3, 2025 at 9:09 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> On 2025-01-29 14:12:52 -0500, Melanie Plageman wrote:
> > From 71f32189aad510b73d221fb0478ffd916e5e5dde Mon Sep 17 00:00:00 2001
> > From: Melanie Plageman <melanieplageman(at)gmail(dot)com>
> > Date: Mon, 27 Jan 2025 12:23:00 -0500
> > Subject: [PATCH v14] Eagerly scan all-visible pages to amortize aggressive
> > vacuum
> >
> > Amortize the cost of an aggressive vacuum by eagerly scanning some
> > all-visible but not all-frozen pages during normal vacuums.
>
> I think it'd be good to explain the problem this is trying to address a bit
> more (i.e. the goal is to avoid the situation which a lot of work is deferred
> until an aggressive vacuum, which is then very expensive).

Attached v15 does this.

> > Because the goal is to freeze these all-visible pages, all-visible pages
> > that are eagerly scanned and set all-frozen in the visibility map are
> > counted as successful eager freezes and those not frozen are considered
> > failed eager freezes.
>
> I don't really understand this sentence. "Because the goal is to freeze these
> all-visible pages" doesn't really relate with the rest of the sentence.

The idea was to motivate why we consider them successes and failures.
Anyway, I removed it.

> > + <varlistentry id="guc-vacuum-max-eager-freeze-failure-rate" xreflabel="vacuum_max_eager_freeze_failure_rate">
> > + <term><varname>vacuum_max_eager_freeze_failure_rate</varname> (<type>floating point</type>)
> > + <indexterm>
> > + <primary><varname>vacuum_max_eager_freeze_failure_rate</varname> configuration parameter</primary>
> > + </indexterm>
> > + </term>
> > + <listitem>
> > + <para>
> > + Specifies the maximum fraction of pages that
> > + <command>VACUUM</command> may scan and <emphasis>fail</emphasis> to set
> > + all-frozen in the visibility map before disabling eager scanning. A
> > + value of <literal>0</literal> disables eager scanning altogether. The
> > + default is <literal>0.03</literal> (3%).
> > + </para>
>
> Fraction of what?

So, as Robert said downthread, it is a fraction of pages. I've changed
the wording of instance of this description to:

"maximum number of pages (as a fraction of total pages in the relation)"

However, I will note that this "fraction of pages" wording appears in
almost every other comment explaining this. v15 does not change these
other occurrences. Do you think I should change them?

> > + <para>
> > + Note that when eager scanning is enabled, successful page freezes
> > + do not count against this limit, although they are internally
> > + capped at 20% of the all-visible but not all-frozen pages in the
> > + relation. Capping successful page freezes helps amortize the
> > + overhead across multiple normal vacuums.
> > + </para>
>
> What does it mean that they are not counted, but are capped?

They are not counted toward the failure cap but they are counted
towards an internally hard-coded success cap. I took a stab at
clarifying this in attached v15.

> > + <para>
> > + If a table is building up a backlog of all-visible but not all-frozen
> > + pages, a normal vacuum may choose to scan skippable pages in an effort to
> > + freeze them. Doing so decreases the number of pages the next aggressive
> > + vacuum must scan. These are referred to as <firstterm>eagerly
> > + scanned</firstterm> pages. Eager scanning can be tuned to attempt
> > + to freeze more all-visible pages by increasing
> > + <xref linkend="guc-vacuum-max-eager-freeze-failure-rate"/>. Even if eager
> > + scanning has kept the number of all-visible but not all-frozen pages to a
> > + minimum, most tables still require periodic aggressive vacuuming.
> > + </para>
>
> Maybe mention that the aggressive vacuuming will often be cheaper than without
> eager freezing, even if necessary?

Done.

> > + * Normal vacuums count all-visible pages eagerly scanned as a success when
> > + * they are able to set them all-frozen in the VM and as a failure when they
> > + * are not able to set them all-frozen.
>
> Maybe some more punctuation would make this more readable? Or a slight
> rephrasing?

Done.

> > + * Because we want to amortize the overhead of freezing pages over multiple
> > + * vacuums, normal vacuums cap the number of successful eager freezes to
> > + * MAX_EAGER_FREEZE_SUCCESS_RATE of the number of all-visible but not
> > + * all-frozen pages at the beginning of the vacuum. Once the success cap has
> > + * been hit, eager scanning is disabled for the remainder of the vacuum of the
> > + * relation.
>
> It also caps the maximum "downside" of freezing eagerly, right? Seems worth
> mentioning.

Done here and one other place (in docs).

> > + /*
> > + * Now calculate the eager scan start block. Start at a random spot
> > + * somewhere within the first eager scan region. This avoids eager
> > + * scanning and failing to freeze the exact same blocks each vacuum of the
> > + * relation.
> > + */
>
> If I understand correctly, we're not really choosing a spot inside the first
> eager scan region, we determine the bounds of the first region?

I'm not sure I understand how those are different, but I updated the
comment a bit. Maybe you can elaborate what you mean?

> > @@ -930,16 +1188,21 @@ lazy_scan_heap(LVRelState *vacrel)
> > vacrel->current_block = InvalidBlockNumber;
> > vacrel->next_unskippable_block = InvalidBlockNumber;
> > vacrel->next_unskippable_allvis = false;
> > + vacrel->next_unskippable_eager_scanned = false;
> > vacrel->next_unskippable_vmbuffer = InvalidBuffer;
> >
> > - while (heap_vac_scan_next_block(vacrel, &blkno, &all_visible_according_to_vm))
> > + while (heap_vac_scan_next_block(vacrel, &blkno, &all_visible_according_to_vm,
> > + &was_eager_scanned))
>
> Pedantic^3: Is past tense really appropriate? We *will* be scanning that page
> in the body of the loop, right?

I thought about this. I chose was_eager_scanned because 1) by the time
we use it, it has been eager scanned and I thought calling it
do_eager_scan might make that less clear and 2) the eager_scanned
logging member of LVRelState is incremented before it is actually
scanned, so I thought that was already set as a precedent.

I haven't changed it in this version, but I am open to renaming it if
you think doing so makes it more clear (especially where the variable
is used [not set]). What do you suggest?

> > @@ -1064,7 +1327,45 @@ lazy_scan_heap(LVRelState *vacrel)
> > if (got_cleanup_lock)
> > lazy_scan_prune(vacrel, buf, blkno, page,
> > vmbuffer, all_visible_according_to_vm,
> > - &has_lpdead_items);
> > + &has_lpdead_items, &vm_page_frozen);
> > +
> > + /*
> > + * Count an eagerly scanned page as a failure or a success.
> > + */
> > + if (was_eager_scanned)
>
> Hm - how should pages be counted that we couldn't get a lock on? I think
> right now they'll be counted as a failure, but that doesn't seem quite right.

Yea, I thought that counting them as failures made sense because we
did fail to freeze them. However, now that you mention it, we didn't
fail to freeze them because of age, so maybe we don't want to count
them as failures. I don't expect us to have a bunch of contended
all-visible pages, so I think the question is about what makes it more
clear in the code. What do you think? Should I reset was_eager_scanned
to false if we don't get the cleanup lock?

> > diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
> > index 0ab921a169b..32a1b8c46a1 100644
> > --- a/src/backend/postmaster/autovacuum.c
> > +++ b/src/backend/postmaster/autovacuum.c
> > @@ -2826,6 +2826,12 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map,
> > tab->at_params.is_wraparound = wraparound;
> > tab->at_params.log_min_duration = log_min_duration;
> > tab->at_params.toast_parent = InvalidOid;
> > +
> > + /*
> > + * Later we check reloptions for vacuum_max_eager_freeze_failure_rate
> > + * override
> > + */
> > + tab->at_params.max_eager_freeze_failure_rate = vacuum_max_eager_freeze_failure_rate;
> > tab->at_storage_param_vac_cost_limit = avopts ?
> > avopts->vacuum_cost_limit : 0;
> > tab->at_storage_param_vac_cost_delay = avopts ?
>
> I'd mention where that is, so that a reader of that comment doesn't have to
> search around...

Done.

- Melanie