Re: pgsql: Fix a couple of bugs in MultiXactId freezing

On Sat, Nov 30, 2013 at 01:06:09AM +0000, Alvaro Herrera wrote:
> Fix a couple of bugs in MultiXactId freezing
>
> Both heap_freeze_tuple() and heap_tuple_needs_freeze() neglected to look
> into a multixact to check the members against cutoff_xid.

> ! /*
> ! * This is a multixact which is not marked LOCK_ONLY, but which
> ! * is newer than the cutoff_multi. If the update_xid is below the
> ! * cutoff_xid point, then we can just freeze the Xmax in the
> ! * tuple, removing it altogether. This seems simple, but there
> ! * are several underlying assumptions:
> ! *
> ! * 1. A tuple marked by an multixact containing a very old
> ! * committed update Xid would have been pruned away by vacuum; we
> ! * wouldn't be freezing this tuple at all.
> ! *
> ! * 2. There cannot possibly be any live locking members remaining
> ! * in the multixact. This is because if they were alive, the
> ! * update's Xid would had been considered, via the lockers'
> ! * snapshot's Xmin, as part the cutoff_xid.

READ COMMITTED transactions can reset MyPgXact->xmin between commands,
defeating that assumption; see SnapshotResetXmin(). I have attached an
isolationtester spec demonstrating the problem. The test spec additionally
covers a (probably-related) assertion failure, new in 9.3.2.

> ! *
> ! * 3. We don't create new MultiXacts via MultiXactIdExpand() that
> ! * include a very old aborted update Xid: in that function we only
> ! * include update Xids corresponding to transactions that are
> ! * committed or in-progress.
> ! */
> ! update_xid = HeapTupleGetUpdateXid(tuple);
> ! if (TransactionIdPrecedes(update_xid, cutoff_xid))
> ! freeze_xmax = true;

That was the only concrete runtime problem I found during a study of the
newest heap_freeze_tuple() and heap_tuple_needs_freeze() code. One thing that
leaves me unsure is the fact that vacuum_set_xid_limits() does no locking to
ensure a consistent result between GetOldestXmin() and GetOldestMultiXactId().
Transactions may start or end between those calls, making the
GetOldestMultiXactId() result represent a later set of transactions than the
GetOldestXmin() result. I suspect that's fine. New transactions have no
immediate effect on either cutoff, and transaction end can only increase a
cutoff. Using a slightly-lower cutoff than the maximum safe cutoff is always
okay; consider vacuum_defer_cleanup_age.

Thanks,
nm

--
Noah Misch
EnterpriseDB http://www.enterprisedb.com