Re: xid wraparound danger due to INDEX_CLEANUP false

On Thu, Apr 16, 2020 at 12:30 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> For btree indexes, IIRC skipping index cleanup could not be a cause of
> corruption, but be a cause of index bloat since it leaves recyclable
> pages which are not marked as recyclable.

I spotted a bug in "Skip full index scan during cleanup of B-tree
indexes when possible" which is unrelated to the index cleanup issue.

This code is wrong, because we don't have a buffer lock (or a buffer
pin) on the buffer anymore:

        ndel = _bt_pagedel(rel, buf);

        /* count only this page, else may double-count parent */
        if (ndel)
        {
            stats->pages_deleted++;
            if (!TransactionIdIsValid(vstate->oldestBtpoXact) ||
                TransactionIdPrecedes(opaque->btpo.xact,
vstate->oldestBtpoXact))
                vstate->oldestBtpoXact = opaque->btpo.xact;
        }

        MemoryContextSwitchTo(oldcontext);
        /* pagedel released buffer, so we shouldn't */

(As the comment says, _bt_pagedel() releases it.)

There is another, more fundamental issue, though: _bt_pagedel() can
delete more than one page. That might be okay if the "extra" pages
were always internal pages, but they're not -- it says so in the
comments above _bt_pagedel(). See the code at the end of
_bt_pagedel(), that says something about why we delete the right
sibling page in some cases.

I think that the fix is to push down the vstate into lower level code
in nbtpage.c. Want to have a go at fixing it?

(It would be nice if we could teach Valgrind to "poison" buffers when
we don't have a pin held...that would probably have caught this issue
almost immediately.)

-- 
Peter Geoghegan