Peter Geoghegan <p...@bowt.ie> writes:
> Any further thoughts on this, Tom?
Sorry for slow response ... my ISP had an equipment failure that took
out my email service for most of a day.
> This is clearly a live bug that we should fix before too long. I could
> write the patch myself, but I would like to get your response to my
> analysis before starting down that road.
Yeah. Looking at the code now, I note these relevant comments in
heapam_index_build_range_scan:
* Also, although our opinions about tuple liveness could change while
* we scan the page (due to concurrent transaction commits/aborts),
* the chain root locations won't, so this info doesn't need to be
* rebuilt after waiting for another transaction.
*
* Note the implied assumption that there is no more than one live
* tuple per HOT-chain --- else we could create more than one index
* entry pointing to the same root tuple.
The core of the issue seems to be that in the presence of concurrent
updates, we might not have a stable opinion of which entry of the HOT
chain is live, leading to trying to index multiple ones of them (using
the same root TID), which leads to the assertion failure.
Also relevant is 1ddc2703a93's commit-log comment that
First, teach IndexBuildHeapScan to not wait for INSERT_IN_PROGRESS or
DELETE_IN_PROGRESS tuples to commit unless the index build is checking
uniqueness/exclusion constraints. If it isn't, there's no harm in just
indexing the in-doubt tuple.
I'm not sure if I was considering the HOT-chain case when I wrote that,
but "no harm" seems clearly wrong in that situation: indexing more than
one in-doubt chain member leads to having multiple index entries pointing
at the same HOT chain. That could be really bad if they have distinct
index values (though we do not expect such a case to arise in a system
catalog, since broken HOT chains should never occur there).
>> Perhaps 2011's commit 520bcd9c9bb missed similar
>> HEAPTUPLE_INSERT_IN_PROGRESS issues that manifest themselves within
>> Justin's test case now?
In the light of this, it bothers me that the DELETE_IN_PROGRESS case
has an exception for HOT chains:
if (checking_uniqueness ||
HeapTupleIsHotUpdated(heapTuple))
// wait
while the INSERT_IN_PROGRESS case has none. Simple symmetry
would suggest that the INSERT_IN_PROGRESS case should be
if (checking_uniqueness ||
HeapTupleIsHeapOnly(heapTuple))
// wait
but I'm not 100% convinced that that's right.
regards, tom lane
