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

On Thu, Apr 16, 2020 at 8:38 AM Andres Freund <andres@anarazel.de> wrote:
>
> 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).

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. The index bloat is the main
side effect of skipping index cleanup. When user executes VACUUM with
INDEX_CLEANUP to reclaim index garbage, such pages will also be
recycled sooner or later? Or skipping index cleanup can be a cause of
recyclable page never being recycled?

Regards,

--
Masahiko Sawada  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services