Re: xid wraparound danger due to INDEX_CLEANUP false

On Fri, May 22, 2020 at 4:40 PM Peter Geoghegan <pg@bowt.ie> wrote:
> On Mon, May 18, 2020 at 7:32 PM Masahiko Sawada
> <masahiko.sawada@2ndquadrant.com> wrote:
> > I've attached WIP patch for HEAD. With this patch, the core pass
> > index_cleanup to bulkdelete and vacuumcleanup callbacks so that they
> > can make decision whether run vacuum or not.
> >
> > If the direction of this patch seems good, I'll change the patch so
> > that we pass something information to these callbacks indicating
> > whether this vacuum is anti-wraparound vacuum. This is necessary
> > because it's enough to invoke index cleanup before XID wraparound as
> > per discussion so far.
>
> It. seems like the right direction to me. Robert?

Sorry, I'm so far behind on my email. Argh.

I think, especially on the blog post you linked, that we should aim to
have INDEX_CLEANUP OFF mode do the minimum possible amount of work
while still keeping us safe against transaction ID wraparound. So if,
for example, it's desirable but not imperative for BRIN to
resummarize, then it's OK normally but should be skipped with
INDEX_CLEANUP OFF.

I find the patch itself confusing and the comments inadequate,
especially the changes to lazy_scan_heap(). Before the INDEX_CLEANUP
patch went into the tree, LVRelStats had a member hasindex indicating
whether or not the table had any indexes. The patch changed that
member to useindex, indicating whether or not we were going to do
index vacuuming; thus, it would be false if either there were no
indexes or if we were going to ignore them. This patch redefines
useindex to mean whether or not the table has any indexes, but without
renaming the variable. There's also really no comments anywhere in the
vacuumlazy.c explaining the overall scheme here; what are we actually
doing? Apparently, what we're really doing here is that even when
INDEX_CLEANUP is OFF, we're still going to keep all the dead tuples.
That seems sad, but if it's what we have to do then it at least needs
comments explaining it.

As for the btree portion of the change, I expect you understand this
better than I do, so I'm reluctant to stick my neck out, but it seems
that what the patch does is force index cleanup to happen even when
INDEX_CLEANUP is OFF provided that the vacuum is for wraparound and
the btree index has at least 1 recyclable page. My first reaction is
to wonder whether that doesn't nerf this feature into oblivion. Isn't
it likely that an index that is being vacuumed for wraparound will
have a recyclable page someplace? If the presence of that 1 recyclable
page causes the "help, I'm about to run out of XIDs, please do the
least possible work" flag to become a no-op, I don't think users are
going to be too happy with that. Maybe I am misunderstanding.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company