On Tue, Apr 9, 2024 at 2:47 AM Robert Haas <robertmhaas@gmail.com> wrote:
>
> - I'm slightly worried about the TID store work (ee1b30f12, 30e144287,
> 667e65aac35), perhaps for no reason. Actually, the results seem really
> impressive,
First, thanks for the complement. I actually suspect if we had this
years ago, it might never have occurred to anyone to go through the
trouble of adding parallel index cleanup.
In a funny way, it's almost too effective -- the claim is that m_w_m
over 1GB is perfectly usable, but I haven't been able to get anywere
near that via vacuum (we have indirectly, via bespoke dev code,
but...). I don't have easy access to hardware that can hold a table
big enough to do so -- vacuuming 1 billion records stays under 400MB.
> and a very quick look at some of the commits seemed kind
> of reassuring. But, like the changes to pruning and freezing, this is
> making some really fundamental changes to critical code. In this case,
> it's code that has evolved very little over the years and seems to
> have now evolved quite a lot.
True. I'd say that at a high level, storage and retrieval of TIDs is a
lot simpler conceptually than other aspects of vacuuming. The
low-level guts are now much more complex, but I'm confident it won't
just output a wrong answer. That aspect has been working for a long
time, and when it has broken during development, it fails very quickly
and obviously.
The more challenging aspects are less cut-and-dried, like memory
management, delegation of responsibility, how to expose locking (which
vacuum doesn't even use), readability/maintainability. Those are more
subjective, but it seems to have finally clicked into place in a way
that feels like the right trade-offs have been made. That's hand-wavy,
I realize.
The more recent follow-up commits are pieces that were discussed and
planned for earlier, but have had less review and were left out from
the initial commits since they're not essential to the functionality.
I judged that test coverage was enough to have confidence in them.
On Tue, Apr 9, 2024 at 6:33 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> This worries me less than other patches like the ones around heap
> changes or the radix tree stuff for TID collection plugged into
> vacuum, which don't have explicit on/off switches AFAIK.
Yes, there is no switch. Interestingly enough, the previous item array
ended up with its own escape hatch to tamp down its eager allocation,
autovacuum_work_mem. Echoing what I said to Robert, if we had the new
storage years ago, I doubt this GUC would ever have been proposed.
I'll also mention the array is still in the code base, but only in a
test module as a standard to test against. Hopefully that offers some
reassurance.