Re: foreign key locks

On 2012-11-14 13:27:26 -0300, Alvaro Herrera wrote:
> Alvaro Herrera wrote:
>
> > > * In heap_lock_tuple's  XMAX_IS_MULTI case
> > >
> > > [snip]
> > >
> > > why is it membermode > mode and not membermode >= mode?
> >
> > Uh, that's a bug.  Fixed.  As noticed in the comment above that snippet,
> > there was a deadlock possible here.  Maybe I should add a test to ensure
> > this doesn't happen.
>
> Done:
> https://github.com/alvherre/postgres/commit/df2847e38198e99f57e52490e1e9391ebb70d770

Some more review bits, based on ffd6250d1d393f2ecb9bfc55c2c6f715dcece557

- if oldestMultiXactId + db is set and then that database is dropped we seem to have a problem because
MultiXactAdvanceOldestwon't overwrite those values. Should probably use SetMultiXactIdLimit directly.
 

- what stop multixacts only being filled out (i.e RecordNewMultiXact()) *after* the XLogInsert() *and* after a
MultiXactGetCheckptMulti()?Afaics MultiXactGenLock is not hold in CreateMultiXactId(). If we crash in that moment we
loosethe multixact data which now means potential data loss...
 

- multixact member group data crossing 512 sector boundaries makes me uneasy (as its 5 bytes). I don't really see a
scenariowhere its dangerous, but ... Does anybody see a problem here?
 

- there are quite some places that domultiStopLimit = multiWrapLimit - 100;if (multiStopLimit < FirstMultiXactId)
multiStopLimit-= FirstMultiXactId;
 
 perhaps MultiXactIdAdvance and MultiXactIdRetreat macros are in order?

- I find using a default: clause in switches with enum types where everything is expected to be handled like the
followinga bad idea, this way the compiler won't warn you if youve missed case's which makes changing/extending code
harder:   switch (rc->strength)    {        case LCS_FORNOKEYUPDATE:            newrc->markType = ROW_MARK_EXCLUSIVE;
        break;        case LCS_FORSHARE:            newrc->markType = ROW_MARK_SHARE;            break;        case
LCS_FORKEYSHARE:           newrc->markType = ROW_MARK_KEYSHARE;            break;        case LCS_FORUPDATE:
newrc->markType= ROW_MARK_KEYEXCLUSIVE;            break;        default:            elog(ERROR, "unsupported rowmark
type%d", rc->strength);    }
 
-
#if 0        /*         * The idea here is to remove the IS_MULTI bit, and replace the         * xmax with the
updater'sXid.  However, we can't really do it:         * modifying the Xmax is not allowed under our buffer locking
   * rules, unless we have an exclusive lock; but we don't know that         * we have it.  So the multi needs to
remainin place :-(         */        ResetMultiHintBit(tuple, buffer, xmax, true);
 
#endif
Three things:   - HeapTupleSatisfiesUpdate is actually always called exclusively locked ;)   - Extending something like
LWLockHeldByMeto also return the current     lockmode doesn't sound hard   - we seem to be using #ifdef NOT_YET for
suchcases
 

- Using a separate production for the lockmode seems to be nicer imo, not really important though
for_locking_item:        FOR UPDATE locked_rels_list opt_nowait
...        | FOR NO KEY UPDATE locked_rels_list opt_nowait
...        | FOR SHARE locked_rels_list opt_nowait
...        | FOR KEY SHARE locked_rels_list opt_nowait    ;

- not really padding, MultiXactStatus is 4bytes.../* * XXX Note: there's a lot of padding space in MultiXactMember.  We
could* find a more compact representation of this Xlog record -- perhaps all the * status flags in one XLogRecData,
thenall the xids in another one?  Not * clear that it's worth the trouble though. */
 
- why
#define SizeOfMultiXactCreate (offsetof(xl_multixact_create, nmembers) + sizeof(int32))
and not
#define SizeOfMultiXactCreate offsetof(xl_multixact_create, members)
- starting a critical section in GetNewMultiXactId but not ending it is ugly, but not new

Greetings,

Andres

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services