Re: Operator families vs. casts

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_indexdef or pg_get_constraintdef,
dropping the object, and recreating it afresh. To implement this optimization
for indexes and index-backed constraints, we need to update the index
definition without perturbing its storage. I found two major ways to do this,
and I'm unsure which will be preferable, so I'm attaching both as alternate
implementations of the same outcome. I'd appreciate feedback on which is
preferable. The first skips the drop and updates pg_index.indclass,
pg_attribute, and pg_constraint.conexclop. The alternate patch retains the
drop and create, then resurrects 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 drilling holes through the performDeletion() and
DefineIndex() stacks to avoid the drop-and-later-preserve dynamic, but that
seemed 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