Re: foreign key locks, 2nd attempt

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.

> > - I haven't done anything about exposing FOR KEY UPDATE to the SQL
> > level. There clearly isn't consensus about exposing this; in fact
> > there isn't consensus on exposing FOR KEY SHARE, but I haven't
> > changed that from the previous patch, either.
>
> I think it would be useful to expose it. Not that anyone should be using
> them in an application (or would it be useful?), but I feel it could
> make testing significantly easier.

Okay, two votes in favor; I'll go do that too.

> > - pg_upgrade bits are missing.
>
> I guess we'll need to rewrite pg_multixact contents in pg_upgrade. Is
> the page format backwards-compatible?

It's not.

I haven't worked out what pg_upgrade needs to do, honestly. I think we
should just not copy old pg_multixact files when upgrading across this
patch. I was initially thinking that pg_multixact should return the
empty set if requested members of a multi that preceded the freeze
point. That way, I thought, we would just never try to access a page
originated in the older version (assuming the freeze point is set to
"current" whenever pg_upgrade runs). However, as things currently
stand, accessing an old multi raises an error. So maybe we need a
scheme a bit more complex to handle this.

> 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.

If we want to keep the original name, it's the same to me.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support