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

Andres Freund wrote:
> On 2013-12-12 18:24:34 -0300, Alvaro Herrera wrote:
> > +            /*
> > +             * It's an update; should we keep it?  If the transaction is known
> > +             * aborted then it's okay to ignore it, otherwise not.  (Note this
> > +             * is just an optimization and not needed for correctness, so it's
> > +             * okay to get this test wrong; for example, in case an updater is
> > +             * crashed, or a running transaction in the process of aborting.)
> > +             */
> > +            if (!TransactionIdDidAbort(members[i].xid))
> > +            {
> > +                newmembers[nnewmembers++] = members[i];
> > +                Assert(!TransactionIdIsValid(update_xid));
> > +
> > +                /*
> > +                 * Tell caller to set HEAP_XMAX_COMMITTED hint while we have
> > +                 * the Xid in cache.  Again, this is just an optimization, so
> > +                 * it's not a problem if the transaction is still running and
> > +                 * in the process of committing.
> > +                 */
> > +                if (TransactionIdDidCommit(update_xid))
> > +                    update_committed = true;
> > +
> > +                update_xid = newmembers[i].xid;
> > +            }
> 
> I still don't think this is ok. Freezing shouldn't set hint bits earlier
> than tqual.c does. What's the problem with adding a
> !TransactionIdIsInProgress()?

I was supposed to tell you, and evidently forgot, that patch 0001 was
the same as previously submitted, and was modified by the subsequent
patches modify per review comments.  These comments should already be
handled in the later patches in the series I just posted.  The idea was
to spare you reading the whole thing all over again, but evidently that
backfired.  I think the new code doesn't suffer from the problem you
mention; and neither the other one that I trimmed out.

> > +    else if (TransactionIdIsValid(update_xid) && !has_lockers)
> > +    {
> > +        /*
> > +         * If there's a single member and it's an update, pass it back alone
> > +         * without creating a new Multi.  (XXX we could do this when there's a
> > +         * single remaining locker, too, but that would complicate the API too
> > +         * much; moreover, the case with the single updater is more
> > +         * interesting, because those are longer-lived.)
> > +         */
> > +        Assert(nnewmembers == 1);
> > +        *flags |= FRM_RETURN_IS_XID;
> > +        if (update_committed)
> > +            *flags |= FRM_MARK_COMMITTED;
> > +        xid = update_xid;
> > +    }
> 
> Afaics this will cause HEAP_KEYS_UPDATED to be reset, is that
> problematic? I don't really remember what it's needed for TBH...

Hmm, will check that out.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services