Re: should check collations when creating partitioned index

On 13.11.23 21:04, Laurenz Albe wrote:
> On Mon, 2023-11-13 at 10:24 +0100, Peter Eisentraut wrote:
>>        * If this table is partitioned and we're creating a unique index, primary
>>        * key, or exclusion constraint, make sure that the partition key is a
>>        * subset of the index's columns.  Otherwise it would be possible to
>>        * violate uniqueness by putting values that ought to be unique in
>>        * different partitions.
>>
>> But this currently doesn't check that the collations between the
>> partition key and the index definition match.  So you can construct a
>> unique index that fails to enforce uniqueness.
>>
>> Here is a partitioned case that doesn't work correctly:
>>
>> create collation case_insensitive (provider=icu,
>> locale='und-u-ks-level2', deterministic=false);
>> create table t1 (a int, b text) partition by hash (b);
>> create table t1a partition of t1 for values with (modulus 2, remainder 0);
>> create table t1b partition of t1 for values with (modulus 2, remainder 1);
>> create unique index i1 on t1 (b collate case_insensitive);
>> insert into t1 values (1, 'a'), (2, 'A');  -- this succeeds
>>
>> The attached patch adds the required collation check.  In the example,
>> it would not allow the index i1 to be created.
> 
> The patch looks good, but I think the error message needs love:
> 
>> +        ereport(ERROR,
>> +            errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
>> +            errmsg("collation of column \"%s\" does not match between partition key and index definition",
>> +                   NameStr(att->attname)));
> 
> "does not match between" sounds weird.  How about
> 
>    collation of index column \"%s\" must match collation of the partitioning key column
> 
> This will be backpatched, right?  What if somebody already created an index like that?
> Does this warrant an entry in the "however" for the release notes, or is the case
> exotic enough that we can assume that nobody is affected?

I think it's exotic enough that I wouldn't bother backpatching it.  But 
I welcome input on this.