Re: nested xacts and phantom Xids

Alvaro Herrera <alvherre@dcc.uchile.cl> writes:
> Here I present the nested transactions patch and the phantom Xids patch
> that goes with it.

I looked at the phantom XIDs stuff a bit.  I still have little confidence
that the concept is correct :-( but here are some comments on the code
level.

> +  * They are marked immediately in pg_subtrans as direct childs of the topmost
> +  * Xid (no matter how deep we are in the transaction tree),

Why is that a good idea --- won't it cause problems when you
commit/abort an intermediate level of subtransaction?

> +  * All this happens when HeapTupleHeaderSetXmax is called and the Xmin of the
> +  * tuple is one of the Xids of the current transaction.
> +  *
> +  * Note that HeapTupleHeaderGetXmax/GetXmin return the masqueraded Xmin and
> +  * Xmax, not the real one in the tuple header.

I think that putting this sort of logic into Set/Get Xmin/Xmax is a
horrid idea.  They are supposed to be transparent fetch/store macros,
not arbitrarily-complex filter functions.  They're also supposed to
be reasonably fast :-(

> +  * ... We know to do this when SetXmax is called
> +  * on a tuple that has the HEAP_MARKED_FOR_UPDATE bit set.

Uglier and uglier.

> ***************
> *** 267,274 ****
>                   return true;
>
>               /* deleting subtransaction aborted */
> !             if (TransactionIdDidAbort(HeapTupleHeaderGetXmax(tuple)))
> !                 return true;
>
>               Assert(TransactionIdIsCurrentTransactionId(HeapTupleHeaderGetXmax(tuple)));
>
> --- 268,276 ----
>                   return true;
>
>               /* deleting subtransaction aborted */
> !             if (tuple->t_infomask & HEAP_XMIN_IS_PHANTOM)
> !                 if (TransactionPhantomIsCommittedPhantom(HeapTupleHeaderGetPhantomId(tuple)))
> !                     return true;
>
>               Assert(TransactionIdIsCurrentTransactionId(HeapTupleHeaderGetXmax(tuple)));
>

Er, what happened to checking for the "deleting subtransaction aborted"
case?

On the whole, I think this was an interesting exercise but also an utter
failure.  We'd be far better off to eat the extra 4 bytes per tuple
header and put back the separate Xmax field.  The costs in both runtime
and complexity/reliability of this patch are simply not justified by
a 4-byte savings.

            regards, tom lane