On Tue, Apr 9, 2024 at 2:47 AM Robert Haas <robertmh...@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 <mich...@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.
