Re: xid wraparound danger due to INDEX_CLEANUP false

Hi,

On 2020-04-29 11:28:00 -0700, Peter Geoghegan wrote:
> On Thu, Apr 16, 2020 at 11:27 AM Andres Freund <andres(at)anarazel(dot)de> wrote:
> > Sure, there is some pre-existing wraparound danger for individual
> > pages. But it's a pretty narrow corner case before INDEX_CLEANUP
> > off.
> >
> > That comment says something about "shared-memory free space map", making
> > it sound like any crash would loose the page. But it's a normal FSM
> > these days. Vacuum will insert the deleted page into the free space
> > map. So either the FSM would need to be corrupted to not find the
> > inserted page anymore, or the index would need to grow slow enough to
> > not use a page before the wraparound. And then such wrapped around xids
> > would exist on individual pages. Not on all deleted pages, like with
> > INDEX_CLEANUP false.
>
> Is that really that narrow, even without "INDEX_CLEANUP false"? It's
> not as if the index needs to grow very slowly to have only very few
> page splits hour to hour (it depends on whether the inserts are random
> or not, and so on). Especially if you had a bulk DELETE affecting many
> rows, which is hardly that uncommon.

Well, you'd need to have a workload that has bulk deletes, high xid
usage *and* doesn't insert new data to use those empty pages

> Fundamentally, btvacuumpage() doesn't freeze 32-bit XIDs (from
> bpto.xact) when it recycles deleted pages. It simply puts them in the
> FSM without changing anything about the page itself. This means
> surprisingly little in the context of nbtree: the
> _bt_page_recyclable() XID check that takes place in btvacuumpage()
> also takes place in _bt_getbuf(), at the point where the page actually
> gets recycled by the client. That's not great.

I think it's quite foolish for btvacuumpage() to not freeze xids. If we
only do so when necessary (i.e. older than a potential new relfrozenxid,
and only when the vacuum didn't yet skip pages), the costs are pretty
miniscule.

> It wouldn't be so unreasonable if btvacuumpage() actually did freeze
> the bpto.xact value at the point where it puts the page in the FSM. It
> doesn't need to be crash safe; it can work as a hint.

I'd much rather make sure the xid is guaranteed to be removed. As
outlined above, the cost would be small, and I think the likelihood of
the consequences of wrapped around xids getting worse over time is
substantial.

> Note that we still need to keep the original bpto.xact XID around for
> _bt_log_reuse_page() (also, do we need to worry _bt_log_reuse_page()
> with a wrapped-around XID?).

I'd just WAL log the reuse when freezing the xid. Then there's no worry
about wraparounds. And I don't think it'd cause additional conflicts;
the vacuum itself (or a prior vacuum) would also have to cause them, I
think?

Greetings,

Andres Freund