Re: foreign key locks, 2nd attempt

On Mon, Jan 16, 2012 at 04:52:36PM -0300, Alvaro Herrera wrote:
> Excerpts from Heikki Linnakangas's message of lun ene 16 16:17:42 -0300 2012:
> > On 15.01.2012 06:49, Alvaro Herrera wrote:
> > > --- 164,178 ----
> > > #define HEAP_HASVARWIDTH 0x0002 /* has variable-width attribute(s) */
> > > #define HEAP_HASEXTERNAL 0x0004 /* has external stored attribute(s) */
> > > #define HEAP_HASOID 0x0008 /* has an object-id field */
> > > ! #define HEAP_XMAX_KEYSHR_LOCK 0x0010 /* xmax is a key-shared locker */
> > > #define HEAP_COMBOCID 0x0020 /* t_cid is a combo cid */
> > > #define HEAP_XMAX_EXCL_LOCK 0x0040 /* xmax is exclusive locker */
> > > ! #define HEAP_XMAX_IS_NOT_UPDATE 0x0080 /* xmax, if valid, is only a locker.
> > > ! * Note this is not set unless
> > > ! * XMAX_IS_MULTI is also set. */
> > > !
> > > ! #define HEAP_LOCK_BITS (HEAP_XMAX_EXCL_LOCK | HEAP_XMAX_IS_NOT_UPDATE | \
> > > ! HEAP_XMAX_KEYSHR_LOCK)
> > > #define HEAP_XMIN_COMMITTED 0x0100 /* t_xmin committed */
> > > #define HEAP_XMIN_INVALID 0x0200 /* t_xmin invalid/aborted */
> > > #define HEAP_XMAX_COMMITTED 0x0400 /* t_xmax committed */
> >
> > HEAP_XMAX_IS_NOT_UPDATE is a pretty opaque name for that. From the name,
> > I'd say that a DELETE would set that, but the comment says it has to do
> > with locking. After going through all the combinations in my mind, I
> > think I finally understood it: HEAP_XMAX_IS_NOT_UPDATE is set if xmax is
> > a multi-xact, that represent only locking xids, no updates. How about
> > calling it "HEAP_XMAX_LOCK_ONLY", and setting it whenever whether or not
> > xmax is a multi-xid?
>
> Hm, sounds like a good idea. Will do.

+1

> > Why are you renaming HeapTupleHeaderGetXmax() into
> > HeapTupleHeaderGetRawXmax()? Any current callers of
> > HeapTupleHeaderGetXmax() should already check that HEAP_XMAX_IS_MULTI is
> > not set.
>
> I had this vague impression that it'd be better to break existing
> callers so that they ensure they decide between
> HeapTupleHeaderGetRawXmax and HeapTupleHeaderGetUpdateXid. Noah
> suggested changing the macro name, too. It's up to each caller to
> decide what's the sematics they want. Most want the latter; and callers
> outside core are more likely to want that one. If we kept the old name,
> they would get the wrong value.

My previous suggestion was to have both macros:

#define HeapTupleHeaderGetXmax(tup) \
( \
AssertMacro(!((tup)->t_infomask & HEAP_XMAX_IS_MULTI)), \
HeapTupleHeaderGetRawXmax(tup) \
)

#define HeapTupleHeaderGetRawXmax(tup) \
( \
(tup)->t_choice.t_heap.t_xmax \
)

No strong preference, though.