On Thu, Jun 25, 2020 at 8:44 PM Peter Geoghegan <pg@bowt.ie> wrote:
> 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.
My opinion is that there's no need to change the code in the
back-branches, and that I don't really like the approach in master
either.
I think what we're saying is that there is no worse consequence to
turning off index_cleanup than some bloat that isn't likely to be
recovered unless you REINDEX. If the problem in question were going to
cause data loss or data corruption or something, we'd have to take
stronger action, but I don't think anyone's saying that this is the
case. Therefore, I think we can handle the back-branches by letting
users know about the bloat hazard and suggesting that they avoid this
option unless it's necessary to avoid running out of XIDs.
Now, what about master? I think it's fine to offer the AM a callback
even when index_cleanup = false, for example so that it can freeze
something in its metapage, but I don't agree with passing it the TIDs.
That seems like it's just inviting it to ignore the emergency brake,
and it's also incurring real overhead, because remembering all those
TIDs can use a lot of memory. If that API limitation causes a problem
for some future index AM, that will be a good point to discuss when
the patch for said AM is submitted for review. I entirely agree with
you that the way btree arranges for btree recycling is crude, and I
would be delighted if you want to improve it, either for v14 or for
any future release, or if somebody else wants to do so. However, even
if that never happens, so what?
In retrospect, I regret committing this patch without better
understanding the issues in this area. That was a fail on my part. At
the same time, it doesn't really sound like the issues are all that
bad. The potential index bloat does suck, but it can still suck less
than the alternatives, and we have evidence that for at least one
user, it was worth a major version upgrade just to replace the
suckitude they had with the suckitude this patch creates.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company