Second thoughts on CheckIndexCompatible() vs. operator families - Mailing list pgsql-hackers

In 367bc426a1c22b9f6badb06cd41fc438fd034639, I introduced a
CheckIndexCompatible() that approves btree and hash indexes having changed to
a different operator class within the same operator family.  To make that
valid, I also tightened the operator family contracts for those access methods
to address casts.  However, I've found two retained formal hazards.  They're
boring and perhaps unlikely to harm real users, but I'd rather nail them down
just in case.


First, opclasses like array_ops have polymorphic opcintype.  Members of such
operator classes could, in general, change behavior arbitrarily in response to
get_fn_expr_argtype().  No core polymorphic operator family member does this.
Nor could they, for no core index access method sets fn_expr.  In the absence
of responses to my previous inquiry[1] on the topic, I'm taking the position
that the lack of fn_expr in these calls is an isolated implementation detail
that could change at any time.  Therefore, we should only preserve an index of
polymorphic operator class when the old and new opcintype match exactly.  This
patch's test suite addition illustrates one ALTER TABLE ALTER TYPE affected by
this new restriction: a conversion from an array type to a domain over that
array type will now require an index rebuild.


Second, as a thought experiment, consider a database with these three types:
1. int4, the core type.
2. int4neg, stores to disk as the negation of its logical value.  Includes a
complete set of operators in the integer_ops btree family, but defines no
casts to other integer_ops-represented types.
3. int4other, stores to disk like int4.  No operators.  Has a implicit binary
coercion cast to int4 and an explicit binary coercion cast to int4neg.

Suppose a table in this database has a column of type int4other with an index
of default operator class.  By virtue of the implicit binary coercion and lack
of other candidates, the index will use int4_ops.  Now, change the type of
that column from int4other to int4neg.  The operator class changes to
int4neg_ops, still within the integer_ops family, and we do not rebuild the
index.  However, the logical sign of each value just flipped, making the index
invalid.  Where did CheckIndexCompatible() miss?  An operator family only
promises cast-compatibility when there exists an implicit or binary coercion
cast between the types in question.  There's no int4->int4neg cast here, not
even a multiple-step pathway.  CheckIndexCompatible() assumes that our ability
to perform a no-rewrite ALTER TABLE ALTER TYPE implies the existence of such a
cast.  It does imply that for the before-and-after _table column types_, but
it's the before-and-after _opcintype_ that matter here.

I think we could close this hazard by having CheckIndexCompatible() test for
an actual implicit or binary coercion cast between the opcintype of the old
and new operator classes.  However, I'm not confident to say that no similar
problem would remain.  Therefore, I propose ceasing to optimize intra-family
index operator transitions and simply requiring exact matches of operator
classes and exclusion operators.  This does not remove any actual optimization
for changes among core types, and it will be simpler to validate.  (I designed
the current rules under the misapprehension that varchar_ops was the default
operator class for varchar columns.  However, varchar_ops is a copy of
text_ops apart from the name and being non-default.  We ship no operator with
a varchar operand, relying instead on binary coercion to text.)

This patch does not, however, remove the new terms from the operator family
contracts.  While core PostgreSQL will no longer depend on them, they may
again prove handy for future optimizations like this.  I remain of the opinion
that they're already widely (perhaps even universally) obeyed.


This patch conflicts trivially with my patch "Avoid FK validations for
no-rewrite ALTER TABLE ALTER TYPE" in that they both add test cases to the
same location in alter_table.sql.  They're otherwise independent, albeit
reflecting parallel principles.

Thanks,
nm

[1] http://archives.postgresql.org/message-id/20111229211711.GA8085@tornado.leadboat.com