Hi,
Thanks, looks saner than my version ;)
On 2013-11-29 01:00:35 -0300, Alvaro Herrera wrote:
> ! /*
> ! * FIXME this calls TransactionIdDidAbort() internally,
> ! * falsifying the claim in the comment at the top ...
> ! */
> ! update_xid = HeapTupleGetUpdateXid(tuple);
Yea. I think we should remove that behaviour from
HeapTupleGetUpdateXid() - we don't need to rely on it here.
> ! /*
> ! * XXX we rely here on HeapTupleGetUpdateXid returning
> ! * Invalid for aborted updates.
> ! */
> ! if (!TransactionIdIsValid(update_xid))
> ! freeze_xmax = true;
I've just added this case because HeapTupleGetUpdateXid() currently can
return InvalidTransactionId - I'd be perfectly fine to just place the
aborted xid in there.
> ! * FIXME -- what if it's a committed update which has recent
> ! * new locker transaction? The tuple wouldn't have been
> ! * removed in that case (HeapTupleSatisfiesVacuum returns
> ! * RECENTLY_DEAD).
> ! */
Afaics we should be protected against that by virtue of GetOldestMultiXactId().
> ***************
> *** 5645,5668 **** heap_tuple_needs_freeze(HeapTupleHeader tuple, TransactionId cutoff_xid,
> ! /* we don't care about lockers */
> ! if (members[i].status != MultiXactStatusNoKeyUpdate ||
> ! members[i].status == MultiXactStatusUpdate)
> ! continue;
Dangerous typo alert. Should also be replaced by
ISUPDATE_from_mxstatus(). Alternatively, if we decide to make
HeapTupleGetUpdateXid() not make that DidAbort() check, we can simply
get rid of doing the loop ourselves alltogethers.
Greetings,
Andres Freund
-- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services
