On 22 August 2014 23:02, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> heap_lock_tuple() has the following comment on top:
>
> * In the failure cases, the routine fills *hufd with the tuple's t_ctid,
> * t_xmax (resolving a possible MultiXact, if necessary), and t_cmax
> * (the last only for HeapTupleSelfUpdated, since we
> * cannot obtain cmax from a combocid generated by another transaction).
> * See comments for struct HeapUpdateFailureData for additional info.
>
> With the patch as submitted, this struct is not filled in the
> HeapTupleWouldBlock case. I'm not sure this is okay, though I admit the
> only caller that passes LockWaitSkip doesn't care, so maybe we could
> just ignore the issue (after properly modifying the comment). It seems
> easy to add a LockBuffer() and "goto failed" rather than a return; that
> would make that failure case conform to HeapTupleUpdated and
> HeapTupleSelfUpdated. OTOH it might be pointless to worry about what
> would be essentially dead code currently ...
Thanks Alvaro.
Forgive me if I have misunderstood but it looks like your incremental
patch included a couple of unrelated changes, namely
s/0/InvalidCommandId/ and a reversion of ConditionalMultiXactIdWait.
Undoing those gave me skip-locked-v12-b.patch, attached for reference,
and I've included those changes in a new full patch
skip-locked-v13.patch (also rebased).
+1 for the change from if-then-else to switch statements.
I was less sure about the 'goto failed' change, but I couldn't measure
any change in concurrent throughput in my simple benchmark, so I guess
that extra buffer lock/unlock doesn't matter and I can see why you
prefer that control flow.
> Did you consider heap_lock_updated_tuple? A rationale for saying it
> doesn't need to pay attention to the wait policy is: if you're trying to
> lock-skip-locked an updated tuple, then you either skip it because its
> updater is running, or you return it because it's no longer running; and
> if you return it, it is not possible for the updater to be locking the
> updated version. However, what if there's a third transaction that
> locked the updated version? It might be difficult to hit this case or
> construct an isolationtester spec file though, since there's a narrow
> window you need to race to.
Hmm. I will look into this, thanks.
Best regards,
Thomas Munro
