xid wraparound danger due to INDEX_CLEANUP false - Mailing list pgsql-hackers

From Andres Freund
Subject xid wraparound danger due to INDEX_CLEANUP false
Date
Msg-id 20200415233848.saqp72pcjv2y6ryi@alap3.anarazel.de
Whole thread Raw
Responses Re: xid wraparound danger due to INDEX_CLEANUP false  (Robert Haas <robertmhaas@gmail.com>)
Re: xid wraparound danger due to INDEX_CLEANUP false  (Masahiko Sawada <sawada.mshk@gmail.com>)
List pgsql-hackers
Hi,

commit a96c41feec6b6616eb9d5baee9a9e08c20533c38
Author: Robert Haas <rhaas@postgresql.org>
Date:   2019-04-04 14:58:53 -0400

    Allow VACUUM to be run with index cleanup disabled.

    This commit adds a new reloption, vacuum_index_cleanup, which
    controls whether index cleanup is performed for a particular
    relation by default.  It also adds a new option to the VACUUM
    command, INDEX_CLEANUP, which can be used to override the
    reloption.  If neither the reloption nor the VACUUM option is
    used, the default is true, as before.

    Masahiko Sawada, reviewed and tested by Nathan Bossart, Alvaro
    Herrera, Kyotaro Horiguchi, Darafei Praliaskouski, and me.
    The wording of the documentation is mostly due to me.

    Discussion: http://postgr.es/m/CAD21AoAt5R3DNUZSjOoXDUY=naYPUOuffVsRzuTYMz29yLzQCA@mail.gmail.com

made the index scan that is part of vacuum optional.  I'm afraid that it
is not safe to do so unconditionally. Until this commit indexes could
rely on at least the amvacuumcleanup callback being called once per
vacuum. Which guaranteed that an index could ensure that there are no
too-old xids anywhere in the index.

But now that's not the case anymore:

    vacrelstats->useindex = (nindexes > 0 &&
                             params->index_cleanup == VACOPT_TERNARY_ENABLED);
...
    /* Do post-vacuum cleanup */
    if (vacrelstats->useindex)
        lazy_cleanup_all_indexes(Irel, indstats, vacrelstats, lps, nindexes);

E.g. btree has xids both in the metapage contents, as well as using it
on normal index pages as part of page deletion.

The slightly oder feature to avoid unnecessary scans during cleanup
protects against this issue by skipping the scan inside the index AM:

/*
 * _bt_vacuum_needs_cleanup() -- Checks if index needs cleanup assuming that
 *            btbulkdelete() wasn't called.
 */
static bool
_bt_vacuum_needs_cleanup(IndexVacuumInfo *info)
{
...
    else if (TransactionIdIsValid(metad->btm_oldest_btpo_xact) &&
             TransactionIdPrecedes(metad->btm_oldest_btpo_xact,
                                   RecentGlobalXmin))
    {
        /*
         * If oldest btpo.xact in the deleted pages is older than
         * RecentGlobalXmin, then at least one deleted page can be recycled.
         */
        result = true;
    }

which will afaict result in all such xids getting removed (or at least
give the AM the choice to do so).


It's possible that something protects against dangers in the case of
INDEX CLEANUP false, or that the consequences aren't too bad. But I
didn't see any comments about the danagers in the patch.

Greetings,

Andres Freund



pgsql-hackers by date:

Previous
From: Cary Huang
Date:
Subject: Re: Include sequence relation support in logical replication
Next
From: Andres Freund
Date:
Subject: Re: Include sequence relation support in logical replication