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

Here is a new patch that addresses all of your review comments. I also
added an example of a problem in the commit message.

On 25.10.24 08:27, jian he wrote:
> + * 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.

both fixed

> /*
> * 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.

all fixed

> + /*
> + * 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).

done

> 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)

Done. I also changed the error message about incompatible types in the
same way, in a separate commit.

> 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");
> }

I don't think this is right. The "old_check_ok" business is only used
when changing an existing foreign key, to see if the check needs to be
run again. The earlier code I add already ensures that if the
collations are nondeterministic, then they have to be the same between
PK and FK. So if this is the case, the only way to change the foreign
key collation is if you have a foreign key that references the same
table and you change the primary key column types in the same ALTER
TABLE statement. Then this conditional ensures that the recheck is run
under appropriate circumstances. But we don't want to error here; the
new situation is valid, we just need to determine whether we have to run
the check again.