Hi, On 2022-02-16 19:43:09 -0800, Peter Geoghegan wrote: > It makes the code in vacuumlazy.c much cleaner. In fact, that's how commit > 44fa8488 started off -- purely as refactoring work.
The problem is that it didn't end up as that. You combined refactoring with substantial changes. And described it large and generic terms. > > 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. It's a contentious thread. I asked for things to be split. In that context, it's imo common curtesy for non-trivial patches to write something like While the rest of the patchset is contentious, I think 0001 is ready to go. I'd like to commit it tomorrow, unless somebody protests. > And it's certainly not a big patch (though it is complicated). For me a vacuum change with the following diffstat is large: 3 files changed, 515 insertions(+), 297 deletions(-) > > 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? Most importantly: On 2021-11-22 11:29:56 -0800, Andres Freund wrote: > I think this is a change mostly in the right direction. But as formulated this > commit does *WAY* too much at once. I do not know how to state more clearly that I think this is not a patch in a committable shape. but also: On 2021-11-22 11:29:56 -0800, Andres Freund wrote: > I think it should be doable to add an isolation test for this path. There have > been quite a few bugs around the wider topic... Greetings, Andres Freund
