Re: Emit fewer vacuum records by reaping removable tuples during pruning

On Fri, Jan 12, 2024 at 2:32 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Fri, Jan 12, 2024 at 1:52 PM Melanie Plageman
> <melanieplageman(at)gmail(dot)com> wrote:
> This analysis seems correct to me, except that "when
> lazy_scan_noprune() is called" should really say "when
> lazy_scan_noprune() is called (and returns true)", because when it
> returns false we fall through and call lazy_scan_prune() afterwards.

Now that I see your patch, I understand what Melanie must have meant.
I agree that there is a small inconsistency here, that we could well
do without.

In general I am in favor of religiously eliminating such
inconsistencies (between lazy_scan_prune and lazy_scan_noprune),
unless there is a reason not to. Not because it's necessarily
important. More because it's just too hard to be sure whether it might
matter. It's usually easier to defensively assume that it matters.

> Here's a draft patch to clean up the inconsistency here. It also gets
> rid of recordfreespace, because ISTM that recordfreespace is adding to
> the confusion here rather than helping anything.

You're using "!prunestate.has_lpdead_items" as part of your test that
sets "recordfreespace". But lazy_scan_noprune doesn't get passed a
pointer to prunestate, so clearly you'll need to detect the same
condition some other way.

--
Peter Geoghegan