Andres Freund wrote: Hi,
> The attached pgbench testcase can reproduce two issues: Thanks. > 2) we frequently error out in heap_lock_updated_tuple_rec() with > ERROR: unable to fetch updated version of tuple > > That's because when we're following a ctid chain, it's perfectly > possible for the updated version of a tuple to already have been > vacuumed/pruned away if the the updating transaction has aborted. > > Also looks like a 9.3+ issues to me, slightly higher impact, but in the > end you're just getting some errors under concurrency. Yes, I think this is 9.3 only. First attachment shows my proposed patch, which is just to report success to caller in case the tuple cannot be acquired by heap_fetch. This is OK because if by the time we check the updated version of a tuple it is gone, it means there is no further update chain to follow and lock. > 1) (takes a bit) > TRAP: FailedAssertion("!(((xid) >= ((TransactionId) 3)))", > File:/pruneheap.c", Line: 601) > > That's because HeapTupleHeaderGetUpdateXid() ignores aborted updaters > and returns InvalidTransactionId in that case, but > HeapTupleSatisfiesVacuum() returns HEAPTUPLE_DELETE_IN_PROGRESS... Interesting. This is a very narrow race condition: when we call HeapTupleSafisfiesVacuum the updater is still running, so it returns HEAPTUPLE_DELETE_IN_PROGRESS; but it aborts just before we read the tuple's update Xid. So that one returns InvalidXid and that causes the failure. I was able to hit this race condition very quickly by adding a pg_usleep(1000) in heap_prune_chain(), in the HEAPTUPLE_DELETE_IN_PROGRESS case, before trying to acquire the update Xid. There is no way to close the window, but there is no need; if the updater aborted, we don't need to mark the page prunable in the first place. So we can just check the return value of HeapTupleHeaderGetUpdateXid and if it's InvalidXid, don't set the prunable bit. The second attachment below fixes the bug that way. I checked for other cases where the update Xid is checked after HeapTupleSatisfiesVacuum returns HEAPTUPLE_DELETE_IN_PROGRESS. As far as I can tell, the only one that would be affected is the one in predicate.c. It is far from clear to me what is the right thing to do in these cases; the simplest idea is to return without reporting a failure if the updater aborted, just as above; but I wonder if this needs to be conditional on "visible". I added a pg_usleep() before acquiring the update Xid in the relevant case, but the isolation test cases didn't hit the problem (I presume there is no update/delete in these test cases, but I didn't check). I defer to Kevin on this issue. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
*** a/src/backend/access/heap/heapam.c --- b/src/backend/access/heap/heapam.c *************** *** 4829,4835 **** heap_lock_updated_tuple_rec(Relation rel, ItemPointer tid, TransactionId xid, ItemPointerCopy(&tupid, &(mytup.t_self)); if (!heap_fetch(rel, SnapshotAny, &mytup, &buf, false, NULL)) ! elog(ERROR, "unable to fetch updated version of tuple"); l4: CHECK_FOR_INTERRUPTS(); --- 4829,4844 ---- ItemPointerCopy(&tupid, &(mytup.t_self)); if (!heap_fetch(rel, SnapshotAny, &mytup, &buf, false, NULL)) ! { ! /* ! * if we fail to find the updated version of the tuple, it's ! * because it was vacuumed/pruned away after its creator ! * transaction aborted. So behave as if we got to the end of the ! * chain, and there's no further tuple to lock: return success to ! * caller. ! */ ! return HeapTupleMayBeUpdated; ! } l4: CHECK_FOR_INTERRUPTS();
*** a/src/backend/access/heap/pruneheap.c --- b/src/backend/access/heap/pruneheap.c *************** *** 479,491 **** heap_prune_chain(Relation relation, Buffer buffer, OffsetNumber rootoffnum, break; case HEAPTUPLE_DELETE_IN_PROGRESS: - /* - * This tuple may soon become DEAD. Update the hint field so - * that the page is reconsidered for pruning in future. - */ - heap_prune_record_prunable(prstate, - HeapTupleHeaderGetUpdateXid(htup)); break; case HEAPTUPLE_LIVE: --- 479,500 ---- break; case HEAPTUPLE_DELETE_IN_PROGRESS: + { + TransactionId xmax; + + /* + * This tuple may soon become DEAD. Update the hint field + * so that the page is reconsidered for pruning in future. + * If there was a MultiXactId updater, and it aborted after + * HTSV checked, then we will get an invalid Xid here. + * There is no need for future pruning of the page in that + * case, so skip it. + */ + xmax = HeapTupleHeaderGetUpdateXid(htup); + if (TransactionIdIsValid(xmax)) + heap_prune_record_prunable(prstate, xmax); + } break; case HEAPTUPLE_LIVE:
*** a/src/backend/storage/lmgr/predicate.c --- b/src/backend/storage/lmgr/predicate.c *************** *** 3907,3912 **** CheckForSerializableConflictOut(bool visible, Relation relation, --- 3907,3914 ---- break; case HEAPTUPLE_DELETE_IN_PROGRESS: xid = HeapTupleHeaderGetUpdateXid(tuple->t_data); + if (!TransactionIdIsValid(xid)) + return; break; case HEAPTUPLE_INSERT_IN_PROGRESS: xid = HeapTupleHeaderGetXmin(tuple->t_data);
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers