Re: MultiXact bugs

On 2013-11-25 13:45:53 -0300, Alvaro Herrera wrote:
> Andres Freund wrote:
> > On 2013-11-25 12:36:19 -0300, Alvaro Herrera wrote:
>
> > > There is no way to close the window, but there is no need; if the
> > > updater aborted, we don't need to mark the page prunable in the first
> > > place. So we can just check the return value of
> > > HeapTupleHeaderGetUpdateXid and if it's InvalidXid, don't set the
> > > prunable bit. The second attachment below fixes the bug that way.
> >
> > I am not sure I like the fact that HeapTupleHeaderGetUpdateXid() checks
> > for aborted transactions in the first place. Why is that a good idea?
> > ISTM that wanders off a fair bit from the other HeapTupleHeaderGet*
> > macros.
>
> Originally it didn't, which caused various bugs. I recall it turned out
> to be cleaner to do the check inside it than putting it out to its
> callers.

This seems strange to me because we do *not* do those checks for
!IS_MULTI xmax's. So there surely shouldn't be too many callers caring
about that?

> I have thoughts that this design might break other things such as the
> priorXmax checking while traversing HOT chains. Not seeing how:

Hm. Are you arguing with yourself about this?

> surely
> if there's an aborted updater in a tuple, there can't be a followup HOT
> chain elsewhere involving the same tuple. A HOT chain would require
> another updater Xid in the MultiXact (and we ensure there can only be
> one updater in a multi). I might be missing something.

I don't see dangers that way either. I think there might be some strange
behaviour because callers need to check IsRunning() first though, which
MultiXactIdGetUpdateXid() doesn't.

Greetings,

Andres Freund

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