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

(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.

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.

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.

--   Heikki Linnakangas  EnterpriseDB   http://www.enterprisedb.com