Re: pgsql: Fix a couple of bugs in MultiXactId freezing
От | Andres Freund |
---|---|
Тема | Re: pgsql: Fix a couple of bugs in MultiXactId freezing |
Дата | |
Msg-id | 20131210005356.GD27840@awork2.anarazel.de обсуждение исходный текст |
Ответ на | Re: pgsql: Fix a couple of bugs in MultiXactId freezing (Alvaro Herrera <alvherre@2ndquadrant.com>) |
Ответы |
Re: pgsql: Fix a couple of bugs in MultiXactId freezing
(Alvaro Herrera <alvherre@2ndquadrant.com>)
|
Список | pgsql-hackers |
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
В списке pgsql-hackers по дате отправления:
Предыдущее
От: Craig RingerДата:
Сообщение: Re: [RFC] Shouldn't we remove annoying FATAL messages from server log?