Re: nested xacts and phantom Xids

Alvaro Herrera <alvherre(at)dcc(dot)uchile(dot)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