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

On Tue, Dec 03, 2013 at 11:56:07AM +0100, Andres Freund wrote:
> On 2013-12-03 00:47:07 -0500, Noah Misch wrote:
> > 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.
> 
> Any idea how to cheat our way out of that one given the current way
> heap_freeze_tuple() works (running on both primary and standby)? My only
> idea was to MultiXactIdWait() if !InRecovery but that's extremly grotty.
> We can't even realistically create a new multixact with fewer members
> with the current format of xl_heap_freeze.

Perhaps set HEAP_XMAX_LOCK_ONLY on the tuple?  We'd then ensure all update XID
consumers check HEAP_XMAX_IS_LOCKED_ONLY() first, much like xmax consumers are
already expected to check HEAP_XMAX_INVALID first.  Seems doable, albeit yet
another injection of complexity.

> > The test spec additionally
> > covers a (probably-related) assertion failure, new in 9.3.2.
> 
> Too bad it's too late to do anthing about it for 9.3.2. :(. At least the
> last seems actually unrelated, I am not sure why it's 9.3.2
> only. Alvaro, are you looking?

(For clarity, the other problem demonstrated by the test spec is also a 9.3.2
regression.)

> > That was the only concrete runtime problem I found during a study of the
> > newest heap_freeze_tuple() and heap_tuple_needs_freeze() code.
> 
> I'd even be interested in fuzzy problems ;). If 9.3. wouldn't have been
> released the interactions between cutoff_xid/multi would have caused me
> to say "back to the drawing" board... I'm not suprised if further things
> are lurking there.

heap_freeze_tuple() of 9.2 had an XXX comment about the possibility of getting
spurious lock contention due to wraparound of the multixact space.  The
comment is gone, and that mechanism no longer poses a threat.  However, a
non-wrapped multixact containing wrapped locker XIDs (we don't freeze locker
XIDs, just updater XIDs) may cause similar spurious contention.

> +                /*
> +                 * The multixact has an update hidden within.  Get rid of it.
> +                 *
> +                 * If the update_xid is below the cutoff_xid, it necessarily
> +                 * must be an aborted transaction.  In a primary server, such
> +                 * an Xmax would have gotten marked invalid by
> +                 * HeapTupleSatisfiesVacuum, but in a replica that is not
> +                 * called before we are, so deal with it in the same way.
> +                 *
> +                 * If not below the cutoff_xid, then the tuple would have been
> +                 * pruned by vacuum, if the update committed long enough ago,
> +                 * and we wouldn't be freezing it; so it's either recently
> +                 * committed, or in-progress.  Deal with this by setting the
> +                 * Xmax to the update Xid directly and remove the IS_MULTI
> +                 * bit.  (We know there cannot be running lockers in this
> +                 * multi, because it's below the cutoff_multi value.)
> +                 */
> +
> +                if (TransactionIdPrecedes(update_xid, cutoff_xid))
> +                {
> +                    Assert(InRecovery || TransactionIdDidAbort(update_xid));
> +                    freeze_xmax = true;
> +                }
> +                else
> +                {
> +                    Assert(InRecovery || !TransactionIdIsInProgress(update_xid));

This assertion is at odds with the comment, but the assertion is okay for now.
If the updater is still in progress, its OldestMemberMXactId[] entry will have
held back cutoff_multi, and we won't be here.  Therefore, if we get here, the
tuple will always be HEAPTUPLE_RECENTLY_DEAD (recently-committed updater) or
HEAPTUPLE_LIVE (aborted updater, recent or not).

Numerous comments in the vicinity (e.g. ones at MultiXactStateData) reflect a
pre-9.3 world.  Most or all of that isn't new with the patch at hand, but it
does complicate study.

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