On Fri, Jan 12, 2024 at 2:32 PM Robert Haas <robertmhaas@gmail.com> wrote:
> On Fri, Jan 12, 2024 at 1:52 PM Melanie Plageman
> <melanieplageman@gmail.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
