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