Re: [COMMITTERS] pgsql: Reduce spurious Hot Standby conflicts from never-visible records

On Sat, 2010-12-11 at 22:03 +0100, Heikki Linnakangas wrote:
> (Moving to pgsql-hackers)
> 
> On 10.12.2010 20:21, Tom Lane wrote:
> > Simon Riggs<simon@2ndQuadrant.com>  writes:
> >> Reduce spurious Hot Standby conflicts from never-visible records.
> >> Hot Standby conflicts only with tuples that were visible at
> >> some point. So ignore tuples from aborted transactions or for
> >> tuples updated/deleted during the inserting transaction when
> >> generating the conflict transaction ids.
> >
> >> Following detailed analysis and test case by Noah Misch.
> >> Original report covered btree delete records, correctly observed
> >> by Heikki Linnakangas that this applies to other cases also.
> >> Fix covers all sources of cleanup records via common code.
> >> Includes additional fix compared to commit on HEAD
> >
> > ISTM HeapTupleHeaderAdvanceLatestRemovedXid is still pretty broken,
> > in that it's examining xmax without having checked that xmax is (a)
> > valid or (b) a lock rather than a deletion xmax.
> 
> In current use, it's only called for tuples that are known to be dead, 
> so either xmax is a valid deletion, or xmin didn't commit in which case 
> the function doesn't use xmax for anything. So I think it actually works 
> as it is.

Well, I think you're both right.

The function shouldn't be called in places where xmax is the wrong
flavour, but there should be specific safeguards in case of mistake.

> I agree it doesn't look right, though. At the very least it needs 
> comments explaining that, but preferably it should do something sane 
> when faced with a tuple that's not dead after all. Perhaps throw an 
> error (though that would be bad during recovery), or an Assert, or just 
> refrain from advancing latestRemovedXid (or advance it, that would be 
> the conservative stance given the current use).

Yes

> Also, I'm not totally convinced it's correct when xmin > xmax, despite 
> Simon's follow-up commit to fix that. Shouldn't it advance 
> latestRemovedXid to xmin in that case? Or maybe it's ok as it is because 
> we know that xmax committed after xmin. The impression I get from the 
> comment above the function now is that it advances latestRemovedXid to 
> the highest XID present in the tuple, but that's not what it does in the 
> xmin > xmax case. That comment needs clarification.

Hmmm, my earlier code took xmax only if xmax > xmin. That was wrong;
what I have now is better, but your point is there may be an even better
truth. I'll think on that a little more.

> While we're at it, perhaps it would be better to move this function to 
> tqual.c. And I feel that a more natural interface would be something like:
> 
> TransactionId
> HeapTupleHeaderGetLatestRemovedXid(HeapTupleHeader tuple);
> 
> IOW, instead bumping up the passed-in latestRemovedXid value, return the 
> highest XID on the tuple (if it was dead).
> 
> PS. it would be good to set hint bits in that function like in 
> HeapTupleSatisfies* functions.

I'm not that happy with refactoring inside a release, plus I'm not even
sure if that is the right way.

I suspect the best way would be to do this as a side-effect of
HeapSatisfiesVacuum(), since this processing should only ever be done in
conjunction with that function.

Will respond later today on those thoughts.

-- Simon Riggs           http://www.2ndQuadrant.com/books/PostgreSQL Development, 24x7 Support, Training and Services