Operator families vs. casts

PostgreSQL 9.1 will implement ALTER TABLE ALTER TYPE operations that use a
binary coercion cast without rewriting the table or unrelated indexes. It
will always rewrite any indexes and recheck any foreign key constraints that
depend on a changing column. This is unnecessary for 100% of core binary
coercion casts. In my original design[1], I planned to detect this by
comparing the operator families of the old and would-be-new indexes. (This
still yields some unnecessary rewrites; oid_ops and int4_ops are actually
compatible, for example.) When I implemented[2] it, I found that the
contracts[3] for operator families are not strong enough to prove that the
existing indexes and constraints remain valid. Specifically, I wished assume
val0 = val1 iff val0::a = val1::b for any val0, val1, a, b such that we
resolve both equality operators in the same operator family. The operator
family contracts say nothing about consistency with casts. Is there a
credible use case for violating that assumption? If not, I'd like to document
it as a requirement for operator family implementors.

The above covers B-tree and hash operator families. GIN and GiST have no
operator family contracts. Here was the comment in my first patch intended to
sweep that under the table:

! * We do not document a contract for GIN or GiST operator families. Only the
! * GIN operator family "array_ops" has more than one constituent operator class,
! * and only typmod-only changes to arrays can avoid a rewrite. Preserving a GIN
! * index across such a change is safe. We therefore support GiST and GIN here
! * using the same rules as for B-tree and hash indexes, but that is mostly
! * academic. Any forthcoming contract for GiST or GIN operator families should,
! * all other things being equal, bolster the validity of this assumption.
! *
! * Exclusion constraints raise the question: can we trust that the operator has
! * the same semantics with the new type? The operator will fall in the index's
! * operator family. For B-tree or hash, the operator will be "=" or "<>",
! * yielding an affirmative answer from contractual requirements. For GiST and
! * GIN, we assume that a similar requirement would fall out of any contract for
! * their operator families, should one arise. We therefore support exclusion
! * constraints without any special treatment, but this is again mostly academic.

Any thoughts on what to do here? We could just add basic operator family
contracts requiring what we need. Perhaps, instead, the ALTER TABLE code
should require an operator family match for B-tree and hash but an operator
class match for other access methods.

For now, I plan to always rewrite indexes on expressions or having predicates.
With effort, we could detect compatible changes there, too.

I also had a more mundane design question in the second paragraph of [2]. It
can probably wait for the review of the next version of the patch. However,
given that it affects a large percentage of the patch, I'd appreciate any
early feedback on it.

Thanks,
nm

[1] http://archives.postgresql.org/message-id/20101229125625.GA27643@tornado.gateway.2wire.net
[2] http://archives.postgresql.org/message-id/20110113230124.GA18733@tornado.gateway.2wire.net
[3] http://www.postgresql.org/docs/9.0/interactive/xindex.html#XINDEX-OPFAMILY

Noah Misch <noah(at)leadboat(dot)com> writes:
> PostgreSQL 9.1 will implement ALTER TABLE ALTER TYPE operations that use a
> binary coercion cast without rewriting the table or unrelated indexes. It
> will always rewrite any indexes and recheck any foreign key constraints that
> depend on a changing column. This is unnecessary for 100% of core binary
> coercion casts. In my original design[1], I planned to detect this by
> comparing the operator families of the old and would-be-new indexes. (This
> still yields some unnecessary rewrites; oid_ops and int4_ops are actually
> compatible, for example.)

No, they aren't: signed and unsigned comparisons do not yield the same
sort order. I think that example may destroy the rest of your argument.

regards, tom lane

On Tue, May 24, 2011 at 10:10:34AM -0400, Tom Lane wrote:
> Noah Misch <noah(at)leadboat(dot)com> writes:
> > PostgreSQL 9.1 will implement ALTER TABLE ALTER TYPE operations that use a
> > binary coercion cast without rewriting the table or unrelated indexes. It
> > will always rewrite any indexes and recheck any foreign key constraints that
> > depend on a changing column. This is unnecessary for 100% of core binary
> > coercion casts. In my original design[1], I planned to detect this by
> > comparing the operator families of the old and would-be-new indexes. (This
> > still yields some unnecessary rewrites; oid_ops and int4_ops are actually
> > compatible, for example.)
>
> No, they aren't: signed and unsigned comparisons do not yield the same
> sort order.

True; scratch the parenthetical comment.

> I think that example may destroy the rest of your argument.

Not that I'm aware of.

Noah,

Providing my thoughts on the 'mundane' question first.

On Tue, May 24, 2011 at 1:40 PM, Noah Misch <noah(at)leadboat(dot)com> wrote:
>
> I also had a more mundane design question in the second paragraph of [2].  It
> can probably wait for the review of the next version of the patch.  However,
> given that it affects a large percentage of the patch, I'd appreciate any
> early feedback on it.

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.

Alexey.

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