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