Re: Operator families vs. casts - Mailing list pgsql-hackers

Alexey,

On Fri, Jun 10, 2011 at 05:46:42PM +0300, Alexey Klyukin wrote:
> Providing my thoughts on the 'mundane' question first.

I was actually referring to this paragraph:
 The standing code handled index/constraint dependencies of changing columns by extracting the SQL definition using
pg_get_indexdefor pg_get_constraintdef, dropping the object, and recreating it afresh.  To implement this optimization
forindexes and index-backed constraints, we need to update the index definition without perturbing its storage.  I
foundtwo major ways to do this, and I'm unsure which will be preferable, so I'm attaching both as alternate
implementationsof the same outcome.  I'd appreciate feedback on which is preferable.  The first skips the drop and
updatespg_index.indclass, pg_attribute, and pg_constraint.conexclop.  The alternate patch retains the drop and create,
thenresurrects the old relfilenode and assigns it to the new object.  The second approach is significantly simpler and
smaller,but it seems less-like anything else we do.  As a further variation on the second approach, I also considered
drillingholes through the performDeletion() and DefineIndex() stacks to avoid the drop-and-later-preserve dynamic, but
thatseemed uglier.
 

However, while we're on the topic you looked at:

> Here's the relevant part of the original post:
> 
> > ATPostAlterTypeCleanup has this comment:
> >         /*
> >          * Re-parse the index and constraint definitions, and attach them to the
> >          * appropriate work queue entries. We do this before dropping because in
> >          * the case of a FOREIGN KEY constraint, we might not yet have exclusive
> >          * lock on the table the constraint is attached to, and we need to get
> >          * that before dropping.  It's safe because the parser won't actually look
> >          * at the catalogs to detect the existing entry.
> >          */
> > Is the second sentence true?  I don't think so, so I deleted it for now.  Here
> > is the sequence of lock requests against the table possessing the FOREIGN KEY
> > constraint when we alter the parent/upstream column:
> >
> > transformAlterTableStmt - ShareRowExclusiveLock
> > ATPostAlterTypeParse - lockmode of original ALTER TABLE
> > RemoveTriggerById() for update trigger - ShareRowExclusiveLock
> > RemoveTriggerById() for insert trigger - ShareRowExclusiveLock
> > RemoveConstraintById() - AccessExclusiveLock
> > CreateTrigger() for insert trigger - ShareRowExclusiveLock
> > CreateTrigger() for update trigger - ShareRowExclusiveLock
> > RI_Initial_Check() - AccessShareLock (3x)
> 
> I think the statement in the second sentence of the comment is true.
> ATPostAlterTypeParse is called only from ATPostAlterTypeCleanup. It has to
> grab the lock on the table the constraint is attached to before dropping the
> constraint. What it does is it opens that relation with the same lock that is
> grabbed for AT_AlterColumnType subtype, i.e. AccessExclusiveLock. I think
> there is no preceding place in AlterTable chain, that grabs stronger lock on
> this relation. ShareRowExclusiveLock is taken in transformAlterTableStmt (1st
> function in your sequence), but this function is ultimately called from
> ATPostAlterTypeParse, just before the latter grabs the AccessExclusiveLock, so
> ultimately at the point where this comment is located no locks are taken for
> the table with a FOREIGN KEY constraint.

The comment is correct that we don't yet have a lock on the remote table at that
point.  But why do we need a lock before RemoveTriggerById() acquires one?
True, it is bad form to get a ShareRowExclusiveLock followed by upgrading to an
AccessExclusiveLock, but the solution there is that RemoveConstraintById() only
needs a ShareRowExclusiveLock.

Granted, in retrospect, I had no business editorializing on this matter.  It's
proximate to the patch's changes but unrelated to them.

Thanks,
nm