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

On 2013-12-09 19:14:58 -0300, Alvaro Herrera wrote:
> Here's a revamped version of this patch.  One thing I didn't do here is
> revert the exporting of CreateMultiXactId, but I don't see any way to
> avoid that.

I don't so much have a problem with exporting CreateMultiXactId(), just
with exporting it under its current name. It's already quirky to have
both MultiXactIdCreate and CreateMultiXactId() in multixact.c but
exporting it imo goes to far.

> Andres mentioned the idea of sharing some code between
> heap_prepare_freeze_tuple and heap_tuple_needs_freeze, but I haven't
> explored that.

My idea would just be to have heap_tuple_needs_freeze() call
heap_prepare_freeze_tuple() and check whether it returns true. Yes,
that's slightly more expensive than the current
heap_tuple_needs_freeze(), but it's only called when we couldn't get a
cleanup lock on a page, so that seems ok.


> ! static TransactionId
> ! FreezeMultiXactId(MultiXactId multi, uint16 t_infomask,
> !                   TransactionId cutoff_xid, MultiXactId cutoff_multi,
> !                   uint16 *flags)
> ! {

> !     if (!MultiXactIdIsValid(multi))
> !     {
> !         /* Ensure infomask bits are appropriately set/reset */
> !         *flags |= FRM_INVALIDATE_XMAX;
> !         return InvalidTransactionId;
> !     }

Maybe comment that we're sure to be only called when IS_MULTI is set,
which will be unset by FRM_INVALIDATE_XMAX? I wondered twice whether we
wouldn't just continually mark the buffer dirty this way.

> !     else if (MultiXactIdPrecedes(multi, cutoff_multi))
> !     {
> !         /*
> !          * This old multi cannot possibly have members still running.  If it
> !          * was a locker only, it can be removed without any further
> !          * consideration; but if it contained an update, we might need to
> !          * preserve it.
> !          */
> !         if (HEAP_XMAX_IS_LOCKED_ONLY(t_infomask))
> !         {
> !             *flags |= FRM_INVALIDATE_XMAX;
> !             return InvalidTransactionId;

Cna you place an Assert(!MultiXactIdIsRunning(multi)) here?

> !         if (ISUPDATE_from_mxstatus(members[i].status) &&
> !             !TransactionIdDidAbort(members[i].xid))#

It makes me wary to see a DidAbort() without a previous InProgress()
call.
Also, after we crashed, doesn't DidAbort() possibly return false for
transactions that were in progress before we crashed? At least that's
how I always understood it, and how tqual.c is written.

> !         /* We only keep lockers if they are still running */
> !         if (TransactionIdIsCurrentTransactionId(members[i].xid) ||

I don't think there's a need to check for
TransactionIdIsCurrentTransactionId() - vacuum can explicitly *not* be
run inside a transaction.


> ***************
> *** 5443,5458 **** heap_freeze_tuple(HeapTupleHeader tuple, TransactionId cutoff_xid,
>                * xvac transaction succeeded.
>                */
>               if (tuple->t_infomask & HEAP_MOVED_OFF)
> !                 HeapTupleHeaderSetXvac(tuple, InvalidTransactionId);
>               else
> !                 HeapTupleHeaderSetXvac(tuple, FrozenTransactionId);
>   
>               /*
>                * Might as well fix the hint bits too; usually XMIN_COMMITTED
>                * will already be set here, but there's a small chance not.
>                */
>               Assert(!(tuple->t_infomask & HEAP_XMIN_INVALID));
> !             tuple->t_infomask |= HEAP_XMIN_COMMITTED;
>               changed = true;
>           }
>       }
> --- 5571,5586 ----
>                * xvac transaction succeeded.
>                */
>               if (tuple->t_infomask & HEAP_MOVED_OFF)
> !                 frz->frzflags |= XLH_FREEZE_XVAC;
>               else
> !                 frz->frzflags |= XLH_INVALID_XVAC;
>   

Hm. Isn't this case inverted? I.e. shouldn't you set XLH_FREEZE_XVAC and
XLH_INVALID_XVAC exactly the other way round? I really don't understand
the moved in/off, since the code has been gone longer than I've followed
the code...

*** a/src/backend/access/rmgrdesc/mxactdesc.c
> --- b/src/backend/access/rmgrdesc/mxactdesc.c
> ***************
> *** 41,47 **** out_member(StringInfo buf, MultiXactMember *member)
>               appendStringInfoString(buf, "(upd) ");
>               break;
>           default:
> !             appendStringInfoString(buf, "(unk) ");
>               break;
>       }
>   }
> --- 41,47 ----
>               appendStringInfoString(buf, "(upd) ");
>               break;
>           default:
> !             appendStringInfo(buf, "(unk) ", member->status);
>               break;
>       }
>   }

That change doesn't look like it will do anything?


Greetings,

Andres Freund

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