Re: altering a column's collation leaves an invalid foreign key

> So for the moment this is a master-only patch. I think once we have
> tied down the behavior we want for the future

/*
* ri_GenerateQualCollation --- add a COLLATE spec to a WHERE clause
*
- * At present, we intentionally do not use this function for RI queries that
- * compare a variable to a $n parameter. Since parameter symbols always have
- * default collation, the effect will be to use the variable's collation.
- * Now that is only strictly correct when testing the referenced column, since
- * the SQL standard specifies that RI comparisons should use the referenced
- * column's collation. However, so long as all collations have the same
- * notion of equality (which they do, because texteq reduces to bitwise
- * equality), there's no visible semantic impact from using the referencing
- * column's collation when testing it, and this is a good thing to do because
- * it lets us use a normal index on the referencing column. However, we do
- * have to use this function when directly comparing the referencing and
- * referenced columns, if they are of different collations; else the parser
- * will fail to resolve the collation to use.
+ * We only have to use this function when directly comparing the referencing
+ * and referenced columns, if they are of different collations; else the
+ * parser will fail to resolve the collation to use. We don't need to use
+ * this function for RI queries that compare a variable to a $n parameter.
+ * Since parameter symbols always have default collation, the effect will be
+ * to use the variable's collation.
+ *
+ * Note that we require that the collations of the referencing and the
+ * referenced column have the some notion of equality: Either they have to
+ * both be deterministic or else they both have to be the same.
*/
" some notion of equality" should it be "same notion of equality"?
maybe we can add "see ATAddForeignKeyConstraint also"
at the end of the whole comment.

/*
* transformColumnNameList - transform list of column names
*
* Lookup each name and return its attnum and, optionally, type OID
*
* Note: the name of this function suggests that it's general-purpose,
* but actually it's only used to look up names appearing in foreign-key
* clauses. The error messages would need work to use it in other cases,
* and perhaps the validity checks as well.
*/
static int
transformColumnNameList(Oid relId, List *colList,
int16 *attnums, Oid *atttypids, Oid *attcollids)
comments need slight adjustment?
comments in transformFkeyGetPrimaryKey also need slight change?
ri_CompareWithCast, we can aslo add comments saying the output is
collation aware.

+ /*
+ * This shouldn't be possible, but better check to make sure we have a
+ * consistent state for the check below.
+ */
+ if ((pkcoll && !fkcoll) || (!pkcoll && fkcoll))
+ elog(ERROR, "key columns are not both collatable");
+
+ if (pkcoll && fkcoll)

cosmetic. pkcoll can change to OidIsValid(pkcoll)
fkcoll change to OidIsValid(fkcoll).

ereport(ERROR,
(errcode(ERRCODE_COLLATION_MISMATCH),
errmsg("foreign key constraint \"%s\" cannot be implemented",
fkconstraint->conname),
errdetail("Key columns \"%s\" and \"%s\" have incompatible
collations: \"%s\" and \"%s\"."
"If either collation is nondeterministic, then both collations
have to be the same.",
strVal(list_nth(fkconstraint->fk_attrs, i)),
strVal(list_nth(fkconstraint->pk_attrs, i)),
get_collation_name(fkcoll),
get_collation_name(pkcoll))));

ERROR: foreign key constraint "a_fk_col1_fkey" cannot be implemented
DETAIL: Key columns "col1" and "col1" have incompatible collations:
"default" and "case_insensitive". If either collation is
nondeterministic, then both collations have to be the same.

"DETAIL: Key columns "col1" and "col1"" may be confusing.
we can be more verbose. like
DETAIL: Key columns "col1" in relation "X" and "col1" in relation "Y"
have incompatible collations......
(just a idea, don't have much preference)

old_check_ok = (new_pathtype == old_pathtype &&
new_castfunc == old_castfunc &&
(!IsPolymorphicType(pfeqop_right) ||
new_fktype == old_fktype) &&
(new_fkcoll == old_fkcoll ||
(get_collation_isdeterministic(old_fkcoll) &&
get_collation_isdeterministic(new_fkcoll))));

I am wondering if one of the new_fkcoll, old_fkcoll is zero, the other is not.
turns out that would n't happen.

before "if (old_check_ok)" we already did extensionsive check that
new_fkcoll is ok
for the job.
in ATAddForeignKeyConstraint, we can leave the old_check_ok part as is.
what do you think of the following:

old_check_ok = (new_pathtype == old_pathtype &&
new_castfunc == old_castfunc &&
(!IsPolymorphicType(pfeqop_right) ||
new_fktype == old_fktype));
if (OidIsValid(new_fkcoll) && OidIsValid(old_fkcoll) &&
new_fkcoll != old_fkcoll)
{
if(!get_collation_isdeterministic(new_fkcoll))
elog(ERROR,"new_fkcoll should be deterministic");
}