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