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

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()?

You also wrote:
On 2013-12-11 22:08:41 -0300, Alvaro Herrera wrote:
> Hmm ... Is there an actual difference?  I mean, a transaction that
> marked itself as committed in pg_clog cannot return to any other state,
> regardless of what happens elsewhere.

Hm, that's not actually true, I missed that so far: Think of async
commits and what we do in tqual.c:SetHintBits(). But I think we're safe
in this scenario, at least for the current callers. vacuumlazy.c will
WAL log the freezing and set the LSN while holding an exclusive lock,
therefor providing an LSN interlock. VACUUM FULL/CLUSTER will be safe,
even with wal_level=minimal, because the relation won't be visible until
it commits and it will contain a smgr pending delete forcing a
synchronous commit. But that should be documented.

> +            if (TransactionIdPrecedes(update_xid, cutoff_xid))
> +            {
> +                update_xid = InvalidTransactionId;
> +                update_committed = false;
> +
> +            }

Deserves an Assert().

> +    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...

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services