Re: Old row version in hot chain become visible after a freeze

________________________________________
From: pgsql-bugs-owner(at)postgresql(dot)org <pgsql-bugs-owner(at)postgresql(dot)org> on behalf of Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
Sent: Wednesday, September 6, 2017 3:12 PM
To: Michael Paquier
Cc: Peter Geoghegan; Wood, Dan; pgsql-bugs(at)postgresql(dot)org
Subject: Re: [BUGS] Old row version in hot chain become visible after a freeze

>Michael Paquier wrote:

>> frame #4: 0x00000001098fba6b postgres`FreezeMultiXactId(multi=34,
>> t_infomask=4930, cutoff_xid=897, cutoff_multi=30,
>> flags=0x00007fff56372fae) + 1179 at heapam.c:6532
>> 6529 * Since the tuple wasn't marked HEAPTUPLE_DEAD by vacuum, the
>> 6530 * update Xid cannot possibly be older than the xid cutoff.
>> 6531 */
>> -> 6532 Assert(!TransactionIdIsValid(update_xid) ||
>> 6533 !TransactionIdPrecedes(update_xid, cutoff_xid));
>> 6534
>> 6535 /*
>> (lldb) p update_xid
>> (TransactionId) $0 = 896
>> (lldb) p cutoff_xid
>> (TransactionId) $1 = 897

>So, looking at this closely, I think there is a bigger problem here: if
>we use any of the proposed patches or approaches, we risk leaving an old
>Xid in a tuple (because of skipping the heap_tuple_prepare_freeze on a
>tuple which remains in the heap with live Xids), followed by later
>truncating pg_multixact / pg_clog removing a segment that might be
>critical to resolving this tuple status later on.

>I think doing the tuple freezing dance for any tuple we don't remove
>from the heap is critical, not optional. Maybe a later HOT pruning
>would save you from actual disaster, but I think it'd be a bad idea to
>rely on that.

There is actually another case where HEAPTUPLE_DEAD tuples may be kept and have
prepare_freeze skipped on them entirely.

lazy_record_dead_tuple may fail to record the heap for later pruning
for lazy_vacuum_heap if there is already a sufficiently large number of dead tuples
in the array:

/*
* The array shouldn't overflow under normal behavior, but perhaps it
* could if we are given a really small maintenance_work_mem. In that
* case, just forget the last few tuples (we'll get 'em next time).
*/
if (vacrelstats->num_dead_tuples < vacrelstats->max_dead_tuples)
...

It looks like we don't even check if the lazy_record_dead_tuple operation actually did any
actual recording in the tupgone case...

if (tupgone)
{
lazy_record_dead_tuple(vacrelstats, &(tuple.t_self));
HeapTupleHeaderAdvanceLatestRemovedXid(tuple.t_data,
&vacrelstats->latestRemovedXid);
tups_vacuumed += 1;
has_dead_tuples = true;
}

It's probably unsafe for any operations calling TruncateXXX code to assume that old Xids don't stick around
after a vacuum in itself.

Thanks,
Yi Wen