Horton

Thread

Commits

  1. Check collation when creating partitioned index

  1. should check collations when creating partitioned index

    Peter Eisentraut <peter@eisentraut.org> — 2023-11-13T09:24:03Z

    When creating a partitioned index, the partition key must be a subset of 
the index's columns.  DefineIndex() explains:

      * 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 non-partitioned case for reference:

create collation case_insensitive (provider=icu, 
locale='und-u-ks-level2', deterministic=false);
create table t0 (a int, b text);
create unique index i0 on t0 (b collate case_insensitive);
insert into t0 values (1, 'a'), (2, 'A');  -- violates unique constraint

Here is a partitioned case that doesn't work correctly:

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.

  2. Re: should check collations when creating partitioned index

    Laurenz Albe <laurenz.albe@cybertec.at> — 2023-11-13T20:04:05Z

    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?

Yours,
Laurenz Albe

  3. Re: should check collations when creating partitioned index

    Peter Eisentraut <peter@eisentraut.org> — 2023-11-14T16:03:18Z

    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.

  4. Re: should check collations when creating partitioned index

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-11-14T16:15:10Z

    Peter Eisentraut <peter@eisentraut.org> writes:
> On 13.11.23 21:04, Laurenz Albe wrote:
>> 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.

I think it should be back-patched.

I don't love the patch details though.  It seems entirely wrong to check
this before we check the opclass match.  Also, in at least some cases
the code presses on looking for another match if the current opclass
doesn't match; you've broken such cases.

			regards, tom lane

  5. Re: should check collations when creating partitioned index

    Jeff Davis <pgsql@j-davis.com> — 2023-11-17T20:02:33Z

    On Mon, 2023-11-13 at 10:24 +0100, Peter Eisentraut wrote:
> 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.

In the patch, you check for an exact collation match. Considering this
case only depends on equality, I think it would be correct if the
requirement was that (a) both collations are deterministic; or (b) the
collations match exactly.

This is related to the discussion here:

https://postgr.es/m/b7a9f32eee8d24518f791168bc6fb653d1f95f4d.camel@j-davis.com

Regards,
	Jeff Davis

  6. Re: should check collations when creating partitioned index

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-11-17T20:18:57Z

    Jeff Davis <pgsql@j-davis.com> writes:
> In the patch, you check for an exact collation match. Considering this
> case only depends on equality, I think it would be correct if the
> requirement was that (a) both collations are deterministic; or (b) the
> collations match exactly.

You keep harping on this idea that we are only concerned with equality,
but I think you are wrong.  We expect a btree index to provide ordering
not only equality, and this example definitely is a btree index.

Possibly, with a great deal more specificity added to the check, we
could distinguish the cases where ordering can't matter and allow
collation variance then.  I do not see the value of that, especially
not when measured against the risk of introducing subtle bugs.

			regards, tom lane

  7. Re: should check collations when creating partitioned index

    Jeff Davis <pgsql@j-davis.com> — 2023-11-17T21:08:09Z

    On Fri, 2023-11-17 at 15:18 -0500, Tom Lane wrote:
> You keep harping on this idea that we are only concerned with
> equality,
> but I think you are wrong.  We expect a btree index to provide
> ordering
> not only equality, and this example definitely is a btree index.
> 
> Possibly, with a great deal more specificity added to the check, we
> could distinguish the cases where ordering can't matter and allow
> collation variance then.  I do not see the value of that, especially
> not when measured against the risk of introducing subtle bugs.

Fair point.

As background, I don't see a complete solution to our collation
problems and on the horizon. You've probably noticed that I'm looking
for various ways to mitigate the problem, and this thread was about
reducing the number of situations in which we rely on collation.

I'll focus on other potential improvements/mitigations and see if I can
make progress somewhere else.

Regards,
	Jeff Davis

  8. Re: should check collations when creating partitioned index

    Peter Eisentraut <peter@eisentraut.org> — 2023-11-20T10:21:41Z

    On 14.11.23 17:15, Tom Lane wrote:
> I don't love the patch details though.  It seems entirely wrong to check
> this before we check the opclass match.

Not sure why?  The order doesn't seem to matter?

> Also, in at least some cases
> the code presses on looking for another match if the current opclass
> doesn't match; you've broken such cases.

I see.  That means we shouldn't raise an error on a mismatch but just do

     if (key->partcollation[i] != collationIds[j])
         continue;

and then let the existing error under if (!found) complain.

I suppose we could move that into the

     if (get_opclass_opfamily_and_input_type(...))

block.  I'm not sure I see the difference.

  9. Re: should check collations when creating partitioned index

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-11-20T16:25:33Z

    Peter Eisentraut <peter@eisentraut.org> writes:
> On 14.11.23 17:15, Tom Lane wrote:
>> I don't love the patch details though.  It seems entirely wrong to check
>> this before we check the opclass match.

> Not sure why?  The order doesn't seem to matter?

The case that was bothering me was if we had a non-collated type
versus a collated type.  That would result in throwing an error
about collation mismatch, when complaining about the opclass seems
more apropos.  However, if we do this:

> I see.  That means we shouldn't raise an error on a mismatch but just do
>      if (key->partcollation[i] != collationIds[j])
>          continue;

it might not matter much.

			regards, tom lane

  10. Re: should check collations when creating partitioned index

    Peter Eisentraut <peter@eisentraut.org> — 2023-11-23T10:01:38Z

    On 20.11.23 17:25, Tom Lane wrote:
> Peter Eisentraut <peter@eisentraut.org> writes:
>> On 14.11.23 17:15, Tom Lane wrote:
>>> I don't love the patch details though.  It seems entirely wrong to check
>>> this before we check the opclass match.
> 
>> Not sure why?  The order doesn't seem to matter?
> 
> The case that was bothering me was if we had a non-collated type
> versus a collated type.  That would result in throwing an error
> about collation mismatch, when complaining about the opclass seems
> more apropos.  However, if we do this:
> 
>> I see.  That means we shouldn't raise an error on a mismatch but just do
>>       if (key->partcollation[i] != collationIds[j])
>>           continue;
> 
> it might not matter much.

Here is an updated patch that works as indicated above.

The behavior if you try to create an index with mismatching collations 
now is that it will skip over the column and complain at the end with 
something like

ERROR:  0A000: unique constraint on partitioned table must include all 
partitioning columns
DETAIL:  UNIQUE constraint on table "t1" lacks column "b" which is part 
of the partition key.

which perhaps isn't intuitive, but I think it would be the same if you 
somehow tried to build an index with different operator classes than the 
partitioning.  I think these less-specific error messages are ok in such 
edge cases.

  11. Re: should check collations when creating partitioned index

    Peter Eisentraut <peter@eisentraut.org> — 2023-11-30T04:50:27Z

    On 23.11.23 11:01, Peter Eisentraut wrote:
> On 20.11.23 17:25, Tom Lane wrote:
>> Peter Eisentraut <peter@eisentraut.org> writes:
>>> On 14.11.23 17:15, Tom Lane wrote:
>>>> I don't love the patch details though.  It seems entirely wrong to 
>>>> check
>>>> this before we check the opclass match.
>>
>>> Not sure why?  The order doesn't seem to matter?
>>
>> The case that was bothering me was if we had a non-collated type
>> versus a collated type.  That would result in throwing an error
>> about collation mismatch, when complaining about the opclass seems
>> more apropos.  However, if we do this:
>>
>>> I see.  That means we shouldn't raise an error on a mismatch but just do
>>>       if (key->partcollation[i] != collationIds[j])
>>>           continue;
>>
>> it might not matter much.
> 
> Here is an updated patch that works as indicated above.
> 
> The behavior if you try to create an index with mismatching collations 
> now is that it will skip over the column and complain at the end with 
> something like
> 
> ERROR:  0A000: unique constraint on partitioned table must include all 
> partitioning columns
> DETAIL:  UNIQUE constraint on table "t1" lacks column "b" which is part 
> of the partition key.
> 
> which perhaps isn't intuitive, but I think it would be the same if you 
> somehow tried to build an index with different operator classes than the 
> partitioning.  I think these less-specific error messages are ok in such 
> edge cases.

If there are no further comments on this patch version, I plan to go 
ahead and commit it soon.

  12. Re: should check collations when creating partitioned index

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-11-30T23:04:57Z

    Peter Eisentraut <peter@eisentraut.org> writes:
>> Here is an updated patch that works as indicated above.
>> 
>> The behavior if you try to create an index with mismatching collations 
>> now is that it will skip over the column and complain at the end with 
>> something like
>> 
>> ERROR:  0A000: unique constraint on partitioned table must include all 
>> partitioning columns
>> DETAIL:  UNIQUE constraint on table "t1" lacks column "b" which is part 
>> of the partition key.
>> 
>> which perhaps isn't intuitive, but I think it would be the same if you 
>> somehow tried to build an index with different operator classes than the 
>> partitioning.  I think these less-specific error messages are ok in such 
>> edge cases.

> If there are no further comments on this patch version, I plan to go 
> ahead and commit it soon.

Sorry for slow response --- I've been dealing with a little too much
$REAL_LIFE lately.  Anyway, I'm content with the v2 patch.  I see
that the existing code works a little harder than this to produce
an on-point error message for mismatching operator, but after
studying that I'm not totally convinced that it's ideal behavior
either.  I think we can wait for some field complaints to see if
we need a better error message for mismatching collation, and
if so what the shape of the bad input is exactly.

			regards, tom lane