On Mon, Jan 27, 2025 at 12:52 PM Masahiko Sawada <sawada.m...@gmail.com> wrote: > > Thank you for updating the patch. I was reviewing the v10 patch and > had some comments. I believe these comments are still valid for v11, > but please ignore them if outdated.
Thanks so much for the review! > + if (TransactionIdIsNormal(vacrel->cutoffs.relfrozenxid) && > + TransactionIdPrecedesOrEquals(vacrel->cutoffs.relfrozenxid, > + > vacrel->cutoffs.FreezeLimit)) > + oldest_unfrozen_before_cutoff = true; > + > + if (!oldest_unfrozen_before_cutoff && > + MultiXactIdIsValid(vacrel->cutoffs.relminmxid) && > + MultiXactIdPrecedesOrEquals(vacrel->cutoffs.relminmxid, > + > vacrel->cutoffs.MultiXactCutoff)) > + oldest_unfrozen_before_cutoff = true; > > Given that our freeze check strictly checks if an xid is older than > the cutoff (exclusive bound), I think we should check if the > relfrozenxid and relminmxid strictly precede the cutoff values. Makes sense. I've changed that. > --- > if (*was_eager_scanned) > vacrel->eager_scanned_pages++; > > How about incrementing this counter near the place where incrementing > scanned_pages (i.e., at the beginning of the loop of > heap_vac_scan_next_block())? It would make it easy to understand the > difference between eager_scanned_pages and scanned_pages. Right. That makes sense. I've changed that in the attached v12. > --- > + * No vm_page_frozen output parameter (like what is passed to > + * lazy_scan_prune()) is passed here because empty pages are always frozen > and > + * thus could never be eagerly scanned. > > The last part "thus could never be eagerly scanned" confused me a bit; > IIUC we count all pages that are scanned because of this new eager > scan feature as "eagerly scanned pages". We increment > eager_scanned_pages counter even if the page is either new or empty. > This fact seems to contradict the sentence "empty pages could never be > eagerly scanned". Ah, so what I mean by this is that the first time an empty page is vacuumed, it is set all-visible and all-frozen in the VM. We only eagerly scan pages that are _only_ all-visible (not also all-frozen). So, no empty pages will ever be eligible for eager scanning. I've updated the comment, because I agree it was confusing. See what you think now. - Melanie
v12-0001-Eagerly-scan-all-visible-pages-to-amortize-aggre.patch
Description: Binary data
