Re: Nonrandom scanned_pages distorts pg_class.reltuples set by VACUUM - Mailing list pgsql-hackers

On Wed, Feb 16, 2022 at 7:08 PM Andres Freund <andres@anarazel.de> wrote:
> I'm quite worried about the pace and style of the vacuum changes. To me it
> doesn't look like that there was design consensus before 44fa8488 was
> committed, quite the opposite (while 44fa8488 probably isn't the center of
> contention, it's not just off to the side either).

I'm not aware of any disagreement whatsoever that is directly related
to 44fa8488. It's true that there was a lot of discussion (even heated
discussion) between myself and Robert, concerning related work on
freezing. But the work in commit 44fa8488 can clearly be justified as
refactoring work. It makes the code in vacuumlazy.c much cleaner. In
fact, that's how commit 44fa8488 started off -- purely as refactoring
work. All of the stuff with freezing (not committed) was started after
the general shape of 44fa8488 was established. One thing followed from
the other.

> You didn't really give a heads up that you're about to commit either. There's
> a note at the bottom of [1], a very long email, talking about committing in a
> couple of weeks. After which there was substantial discussion of the patchset.

How can you be surprised that I committed 44fa8488? It's essentially
the same patch as the first version, posted November 22 -- almost 3
months ago. And it's certainly not a big patch (though it is
complicated).

How was 44fa8488 substantively different to v1 of the patch, posted
all the way back on November 22? Literally the only thing that changed
is that it became more polished, based in part on your feedback.
That's it!

> It doesn't look to me like there was a lot of review for 44fa8488, despite it
> touching very critical parts of the code. I'm listed as a reviewer, that was a
> few months ago, and rereading my review I don't think it can be read to agree
> with the state of the code back then.

Can you be more specific?

> I also have a hard time making heads or tails out of the commit message of
> 44fa84881ff. It's quite long without being particularly descriptive. The
> commit just changes a lot of things at once, making it hard to precisely
> describe and very hard to review and understand.

The commit message is high level. The important point is that we can
more or less treat all scanned_pages the same, without generally
caring about whether or not a cleanup lock could be acquired (since
the exceptions where that isn't quite true are narrow and rare). That
is the common theme, for everything.

I do recall that you said something about the patch that became commit
44fa8488 being too much all at once. In particular, something about
removing FORCE_CHECK_PAGE() in a separate commit. But I don't think
that that made sense -- which I said at the time. There were subtle
interactions between that and between the other stuff -- which is
actually what led to this 74388a1ac36 fix-up. But as I said in the
commit message to 74388a1ac36, I think that this issue might have been
latent. If it wasn't there all along, then it might have been. Sort of
like the issue reference in 2011 commit b4b6923e.

-- 
Peter Geoghegan