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

Hi,

Thank you for working on this!

On Sat, 14 Dec 2024 at 01:53, Melanie Plageman
<melanieplageman(at)gmail(dot)com> wrote:
>
> On Thu, Nov 7, 2024 at 10:42 AM Andres Freund <andres(at)anarazel(dot)de> wrote:
> >
> > Hi,
>
> Thanks for the review!
> Attached v2 should address your feedback and also fixes a few bugs with v1.

Here are couple of code comments:

=== [PATCH v2 07/10] ===

It took me a while to understand that heap_vac_scan_next_block() loops
until rel_pages. What do you think about adding
Assert(vacrel->current_block == rel_pages) before the
pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_SCANNED,
rel_pages) and having a comment on main loop should process blocks
until rel_pages?

=== [PATCH v2 09/10] ===

+ * If there are no indexes or index scanning is disabled, phase II may be
+ * skipped. If phase I identified very few dead index entries, vacuum may skip
+ * phases II and III. Index vacuuming deletes the dead index entries from the
+ * TID store.

phase III is not mentioned in the previous comments. It could be
better to first explain what phase III is before mentioning it.

+ * After index vacuuming is complete, vacuum scans the blocks of the relation
+ * indicated by the TIDs in the TID store and reaps the dead tuples, freeing
+ * that space for future tuples. Finally, vacuum may truncate the relation if
+ * it has emptied pages at the end.
+ *
+ * After finishing all phases of work, vacuum updates relation statistics in
+ * pg_class and the cumulative statistics subsystem.

There is no clear definition of phase III here as well. I can not
understand what phase III is and which parts the vacuum may skip.

=== [PATCH v2 10/10] ===

+ /*
+ * The number of eagerly scanned blocks an eager vacuum failed to
+ * freeze (due to age) in the current eager scan region. Eager vacuums
+ * reset it to EAGER_SCAN_MAX_FAILS_PER_REGION each time they enter a
+ * new region of the relation. Aggressive vacuums also decrement this
+ * coutner but it is initialized to # blocks in the relation.
+ */

s/coutner/counter

/* non-export function prototypes */
+
static void lazy_scan_heap(LVRelState *vacrel);

Extra blank line.

+ if (TransactionIdIsNormal(vacrel->cutoffs.relfrozenxid) &&
+ TransactionIdPrecedesOrEquals(vacrel->cutoffs.relfrozenxid,
+ vacrel->cutoffs.FreezeLimit))
+ oldest_unfrozen_requires_freeze = true;
+
+ if (MultiXactIdIsValid(vacrel->cutoffs.relminmxid) &&
+ MultiXactIdPrecedesOrEquals(vacrel->cutoffs.relminmxid,
+ vacrel->cutoffs.MultiXactCutoff))
+ oldest_unfrozen_requires_freeze = true;

You may want to short-circuit the second if condition with
!oldest_unfrozen_requires_freeze but it may increase the complexity,
up to you.

+ vacrel->eager_pages.remaining_fails = EAGER_SCAN_MAX_FAILS_PER_REGION *
+ (1 - vacrel->next_eager_scan_region_start / EAGER_SCAN_REGION_SIZE);

This will always return EAGER_SCAN_MAX_FAILS_PER_REGION or 0 because
of the integer dividing.

+ if (aggressive)
+ {
+ vacrel->eagerness = VAC_AGGRESSIVE;
+
+ /*
+ * An aggressive vacuum must scan every all-visible page to safely
+ * advance the relfrozenxid and/or relminmxid. As such, there is no
+ * cap to the number of allowed successes or failures.
+ */
+ vacrel->eager_pages.remaining_fails = vacrel->rel_pages + 1;
+ vacrel->eager_pages.remaining_successes = vacrel->rel_pages + 1;
+ return;
+ }
...
...
+ if (was_eager_scanned)
+ {
+ if (vm_page_frozen)
+ {
+ Assert(vacrel->eager_pages.remaining_successes > 0);
+ vacrel->eager_pages.remaining_successes--;
+
+ if (vacrel->eager_pages.remaining_successes == 0)
+ {
+ Assert(vacrel->eagerness == VAC_EAGER);

My initial thought was that since *was_eager_scanned* is true,
Assert(vacrel->eagerness == VAC_EAGER) should be under the top if
condition (I assumed that was_eager_scanned is only true for eager
vacuums, not for aggressive vacuums too) but I see your point here.
Since you set vacrel->eager_pages.remaining_successes to
vacrel->rel_pages + 1, vacrel->eager_pages.remaining_successes can not
reach 0 although all pages are processed as successful. I think
comment is needed in both places to explain why
vacrel->eager_pages.[remaining_fails | remaining_successes] is set to
vacrel->rel_pages + 1 and why vacrel->eagerness should be VAC_EAGER
when was_eager_scanned is true and
vacrel->eager_pages.remaining_successes is 0.

--
Regards,
Nazir Bilal Yavuz
Microsoft