Skip site navigation (1) Skip section navigation (2)

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

From: Simon Riggs <simon(at)2ndQuadrant(dot)com>
To: Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [COMMITTERS] pgsql: Reduce spurious Hot Standby conflicts from never-visible records
Date: 2010-12-12 10:15:40
Message-ID: 1292148940.2737.1530.camel@ebony (view raw or whole thread)
Lists: pgsql-committerspgsql-hackers
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(at)2ndQuadrant(dot)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).


> 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 
 PostgreSQL Development, 24x7 Support, Training and Services

In response to


pgsql-hackers by date

Next:From: Alexander KorotkovDate: 2010-12-12 11:27:25
Subject: Re: Wildcard search support for pg_trgm
Previous:From: David E. WheelerDate: 2010-12-12 03:35:33
Subject: Re: function attributes

pgsql-committers by date

Next:From: Tom LaneDate: 2010-12-12 16:16:46
Subject: Re: [COMMITTERS] pgsql: Use symbolic names not octal constants for file permission flags
Previous:From: Heikki LinnakangasDate: 2010-12-11 21:03:23
Subject: Re: [COMMITTERS] pgsql: Reduce spurious Hot Standby conflicts from never-visible records

Privacy Policy | About PostgreSQL
Copyright © 1996-2015 The PostgreSQL Global Development Group