On Thu, Jun 25, 2020 at 8:28 AM Robert Haas <robertmh...@gmail.com> wrote: > I'm not really convinced. I agree that from a theoretical point of > view an index can have arbitrary needs and is the arbiter of its own > needs, but when I pull the emergency break, I want the vehicle to > stop, not think about stopping.
Making this theoretical argument in the context of this discussion was probably a mistake. I agree that this is the emergency break, and it needs to work like one. It might be worth considering some compromise in the event of using the "vacuum_index_cleanup" reloption (i.e. when the user has set it to 'off'), provided there is good reason to believe that we're not in an emergency -- I mentioned this to Masahiko just now. I admit that that isn't very appealing for other reasons, but it's worth considering a way of ameliorating the problem in back branches. We really ought to change how recycling works, so that it happens at the tail end of the same VACUUM operation that deleted the pages -- but that cannot be backpatched. It might be that the most appropriate mitigation in the back branches is a log message that reports on the fact that we've probably leaked pages due to this issue. Plus some documentation. Though even that would require calling nbtree to check if that is actually true (by checking the metapage), so it still requires backpatching something close to Masahiko's draft patch. > I don't think I believe this. All you need is one small range-deletion, right? Right. > > Then there's question 2. The intuition behind the approach from > > Sawada-san's patch was that allowing wraparound here feels wrong -- it > > should be in the AM's hands. However, it's not like I can point to > > some ironclad guarantee about not leaking deleted pages that existed > > before the INDEX_CLEANUP feature. We know that the FSM is not crash > > safe, and that's that. Is this really all that different? Maybe it is, > > but it seems like a quantitative difference to me. > > I don't think I believe this, either. In the real-world example to > which you linked, the user ran REINDEX afterward to recover from index > bloat, and we could advise other people who use this option that it > may leak space that a subsequent VACUUM may fail to recover, and > therefore they too should consider REINDEX. I was talking about the intuition behind the design. I did not intend to suggest that nbtree should ignore "INDEX_CLEANUP = off" regardless of the consequences. I am sure about this much: The design embodied by Masahiko's patch is clearly a better one overall, even if it doesn't fix the problem on its own. I agree that we cannot allow nbtree to ignore "INDEX_CLEANUP = off", even if that means leaking pages that could otherwise be recycled. I'm not sure what we should do about any of this in the back branches, though. I wish I had a simple idea about what to do there. -- Peter Geoghegan
