xid wraparound danger due to INDEX_CLEANUP false

Hi,

commit a96c41feec6b6616eb9d5baee9a9e08c20533c38
Author: Robert Haas <rhaas(at)postgresql(dot)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