Re: BUG #17409: Unable to alter data type of clustered column which is referenced by foreign key

Japin Li <japinli@hotmail.com> writes:
> Attached v4 patch, please consider this for futher review.

I was about ready to push this, when I started to wonder why the
problem was introduced by 8b069ef5d, which on its face is merely
a small performance improvement.  After digging into it, I find
that in fact, that changed the semantics of get_constraint_index(),
and not trivially so.  In the cases at hand, ATExecAlterColumnType's
scan of pg_depend finds two entries for constraints that need to
be rebuilt: there is the pk_atref primary key constraint, and there
is the fk_atref foreign key constraint.  The new implementation
of get_constraint_index() returns the pk_atref index as the
associated index of both constraints, whereas the old code returned
it only for the pk_atref constraint.  The old behavior is what
ATExecAlterColumnType wants, I judge.  As this stands, we will
rebuild indexes that don't need to be rebuilt, and indeed might
be on other tables altogether from the one that is being modified
(which opens all sorts of potential locking problems).  So I think
what we actually want to do is not this, but to revert that
behavioral change.  As far as I can see, the other sites that
call get_constraint_index() only do so on constraint types that
really own indexes; but there might be external code that expects
get_constraint_index() to have its old behavior.

The fundamental problem is that get_constraint_index() is no
longer satisfying its API spec:

 *        Given the OID of a unique, primary-key, or exclusion constraint,
 *        return the OID of the underlying index.

as now it will also return a nonzero OID for FK constraints (and
maybe other kinds?).

One way to fix this is to add a check of contype to
get_constraint_index(); the other way is to do so at the
call sites.  I'm kind of leaning to the former for v14,
but since it makes get_constraint_index() less generally
useful, maybe we should change its API spec in HEAD?
Not sure.  Checking contype separately would require an
additional syscache lookup, so it's not *that* attractive.

            regards, tom lane