On Thu, Jun 25, 2020 at 8:28 AM Robert Haas <robertmhaas@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