Обсуждение: Potential (low likelihood) wraparound hazard inheap_abort_speculative()

Hi,

heap_abort_speculative() does:
    /*
     * The tuple will become DEAD immediately.  Flag that this page
     * immediately is a candidate for pruning by setting xmin to
     * RecentGlobalXmin.  That's not pretty, but it doesn't seem worth
     * inventing a nicer API for this.
     */
    Assert(TransactionIdIsValid(RecentGlobalXmin));
    PageSetPrunable(page, RecentGlobalXmin);

but that doesn't seem right to me. RecentGlobalXmin could very well be
older than the table's relfrozenxid. Especially when multiple databases
are used, or logical replication is active, it's not unlikely at all.

That's because RecentGlobalXmin is a) the minimum xmin of all databases,
whereas horizon computations for relations are done for only the current
database b) RecentGlobalXmin may have been computed a while ago (when
the snapshot for the transaction was computed, for example), but a
concurrent vacuum could be more recent c) RecentGlobalXmin includes the
more "pessimistic" xmin for catalog relations.

Unless somebody has a better idea for how to solve this in a
back-paptchable way, I think it'd be best to replace RecentGlobalXmin
with RecentXmin. That'd be safe as far as I can see.

Greetings,

Andres Freund

On Sat, Mar 28, 2020 at 2:30 PM Andres Freund <andres@anarazel.de> wrote:
> Unless somebody has a better idea for how to solve this in a
> back-paptchable way, I think it'd be best to replace RecentGlobalXmin
> with RecentXmin. That'd be safe as far as I can see.

As far as I can tell, the worst consequence of this wraparound hazard
is that we don't opportunistically prune at some later point where we
probably ought to. Do you agree with that assessment?

Since pd_prune_xid is documented as "a hint field" in bufpage.h, this
bug cannot possibly lead to queries that give wrong answers. The
performance issue also seems like it should not have much impact,
since we only call heap_abort_speculative() in extreme cases where
there is a lot of contention among concurrent upserting sessions.
Also, as you pointed out already, RecentGlobalXmin is probably not
going to be any different to RecentXmin.

I am in favor of fixing the issue, and backpatching all the way. I
just want to put the issue in perspective, and have my own
understanding of things verified.

-- 
Peter Geoghegan

Hi,

On 2020-03-29 15:20:01 -0700, Peter Geoghegan wrote:
> On Sat, Mar 28, 2020 at 2:30 PM Andres Freund <andres@anarazel.de> wrote:
> > Unless somebody has a better idea for how to solve this in a
> > back-paptchable way, I think it'd be best to replace RecentGlobalXmin
> > with RecentXmin. That'd be safe as far as I can see.
> 
> As far as I can tell, the worst consequence of this wraparound hazard
> is that we don't opportunistically prune at some later point where we
> probably ought to. Do you agree with that assessment?

Probably, yes.


> Since pd_prune_xid is documented as "a hint field" in bufpage.h, this
> bug cannot possibly lead to queries that give wrong answers. The
> performance issue also seems like it should not have much impact,
> since we only call heap_abort_speculative() in extreme cases where
> there is a lot of contention among concurrent upserting sessions.

Well, I think it could be fairly "persistent" in being set in some
cases. PageSetPrunable() and heap_prune_record_prunable() check that a
new prune xid is newer than the current one.

That said, I still think it's unlikely to be really problematic.



> Also, as you pointed out already, RecentGlobalXmin is probably not
> going to be any different to RecentXmin.

Huh, I think they very commonly are radically different? Where did I
point that out? RecentXmin is the xmin of the last snapshot
computed. Whereas RecentGlobalXmin basically is the oldest xmin of any
backend. That's a pretty large difference? Especially with longrunning
sessions, replication, logical decoding those can be different by
hundreds of millions of xids.

Where did I point out that they're not going to be very different?


> I am in favor of fixing the issue, and backpatching all the way. I
> just want to put the issue in perspective, and have my own
> understanding of things verified.

Cool.

Greetings,

Andres Freund