Re: New IndexAM API controlling index vacuum strategies - Mailing list pgsql-hackers

On Mon, Mar 29, 2021 at 12:16 AM Peter Geoghegan <pg@bowt.ie> wrote:
> And now here's v8, which has the following additional cleanup:

I can't effectively review 0001 because it both changes the code for
individual functions significantly and reorders them within the file.
I think it needs to be separated into two patches, one of which makes
the changes and the other of which reorders stuff. I would probably
vote for just dropping the second one, since I'm not sure there's
really enough value there to justify the code churn, but if we're
going to do it, I think it should definitely be done separately.

Here are a few comments on the parts I was able to understand:

* "onerel" is a stupid naming convention that I'd rather not propagate
further. It makes sense in the context of a function whose job it is
to iterate over a list of relations and do something for each one. But
once you're down into the code that only knows about one relation in
the first place, calling that relation "onerel" rather than "rel" or
"vacrel" or even "heaprel" is just confusing. Ditto for "onerelid".

* Moving stuff from static variables into LVRelState seems like a
great idea. Renaming it from LVRelStats seems like a good idea, too.

* Setting vacrel->lps = NULL "for now" when we already did palloc0 at
allocation time seems counterproductive.

* The code associated with the comment block that says "Initialize
state for a parallel vacuum" has been moved inside lazy_space_alloc().
That doesn't seem like an especially good choice, because no casual
reader is going to expect a function called lazy_space_alloc() to be
entering parallel mode and so forth as a side effect. Also, the call
to lazy_space_alloc() still has a comment that says "Allocate the
space for dead tuples in case parallel vacuum is not initialized."
even though the ParallelVacuumIsActive() check has been removed and
the function now does a lot more than allocating space.

* lazy_scan_heap() removes the comment which begins "Note that
vacrelstats->dead_tuples could have tuples which became dead after
HOT-pruning but are not marked dead yet." But IIUC that special case
is removed by a later patch, not 0001, in which case it is that patch
that should be touching this comment.

Regarding 0002:

* It took me a while to understand why lazy_scan_new_page() and
lazy_scan_empty_page() are named the way they are. I'm not sure
exactly what would be better, so I am not necessarily saying I think
you have to change anything, but for the record I think this naming
sucks. The reason we have "lazy" in here, AFAIU, is because originally
we only had old-style VACUUM FULL, and that was the good hard-working
VACUUM, and what we now think of as VACUUM was the "lazy" version that
didn't really do the whole job. Then we decided it was the
hard-working version that actually sucked and we always wanted to be
lazy (or else rewrite the table). So now we have all of these
functions named "lazy" which are really just functions to do "vacuum".
But, if we just did s/lazy/vacuum/g we'd be in trouble, because we use
"vacuum" to mean "part of vacuum." That's actually a pretty insane
thing to do, but we like terminological confusion so much that we
decided to use the word vacuum not just to refer to one part of vacuum
but to two different parts of vacuum. During heap vacuuming, which is
the relevant thing here, we call the first part a "scan" and the
second part "vacuum," hence lazy_scan_page() and lazy_vacuum_page().
For indexes, we can decide to vacuum indexes or cleanup indexes,
either of which is part of our overall strategy of trying to do a
VACUUM. We need some words here that are not so overloaded. If, for
example, we could agree that the whole thing is vacuum and the first
time we touch the heap page that's the strawberry phase and then the
second time we touch it that's the rhubarb phase, then we could have
vacuum_strawberry_page(), vacuum_strawberry_new_page(),
vacuum_rhubarb_phase(), etc. and everything would be a lot clearer,
assuming that you replaced the words "strawberry" and "rhubarb" with
something actually meaningful. But that seems hard. I thought about
suggesting that the word for strawberry should be "prune", but it does
more than that. I thought about suggesting that either the word for
strawberry or the word for rhubarb should be "cleanup," but that's
another word that is already confusingly overloaded. So I don't know.

* But all that having been said, it's easy to get confused and think
that lazy_scan_new_page() is scanning a new page for lazy vacuum, but
in fact it's the new-page handler for the scan phase of lazy vacuum,
and it doesn't scan anything at all. If there's a way to avoid that
kind of confusion, +1 from me.

* One possibility is that maybe it's not such a great idea to put this
logic in its own function. I'm rather suspicious on principle of
functions that are called with a locked or pinned buffer and release
the lock or pin before returning. It suggests that the abstraction is
not very clean. A related problem is that, post-refactoring, the
parallels between the page-is-new and page-is-empty cases are harder
to spot. Both at least maybe do RecordPageWithFreeSpace(), both do
UnlockReleaseBuffer(), etc. but you have to look at the subroutines to
figure that out after these changes. I understand the value of keeping
the main function shorter, but it doesn't help much if you have to go
jump into all of the subroutines and read them anyway.

* The new comment added which begins "Even if we skipped heap vacuum,
..." is good, but perhaps it could be more optimistic. It seems to me
that it's not just that it *could* be worthwhile because we *could*
have updated freespace, but that those things are in fact probable.

* I'm not really a huge fan of comments that include step numbers,
because they tend to cause future patches to have to change a bunch of
comments every time somebody adds a new step, or, less commonly,
removes an old one. I would suggest revising the comments you've added
that say things like "Step N for block: X" to just "X". I do like the
comment additions, just not the attributing of specific numbers to
specific steps.

* As in 0001, core logical changes are obscured by moving code and
changing it in the same patch. All this logic gets moved into
lazy_scan_prune() and revised at the same time. Using git diff
--color-moved -w sorta works, but even then there are parts of it that
are pretty hard to read, because there's a bunch of other stuff that
gets rejiggered at the same time.

My concentration is flagging a bit so I'm going to stop reviewing here
for now. I'm not deeply opposed to any of what I've seen so far. My
main criticism is that I think more thought should be given to how
things are named and to separating minimal code-movement patches from
other changes.

Thanks,

-- 
Robert Haas
EDB: http://www.enterprisedb.com