Thread: [PATCH] Check operator when creating unique index on partition table

Hi,

I found that things could go wrong in some cases, when the unique index and the partition key use different opclass.

For example:
```
CREATE TABLE ptop_test (a int, b int, c int) PARTITION BY LIST (a);
CREATE TABLE ptop_test_p1 PARTITION OF ptop_test FOR VALUES IN ('1');
CREATE TABLE ptop_test_m1 PARTITION OF ptop_test FOR VALUES IN ('-1');
CREATE UNIQUE INDEX ptop_test_unq_abs_a ON ptop_test (a abs_int_btree_ops);  -- this should fail
```
In this example, `abs_int_btree_ops` is a opclass whose equality operator is customized to consider ‘-1’ and ‘1’ as
equal.
So the unique index should not be allowed to create, since ‘-1’ and ‘1’ will be put in different partition.

The attached patch should fix this problem.


Best Regards,
Guancheng Luo

Guancheng Luo <prajnamort@gmail.com> writes:
> I found that things could go wrong in some cases, when the unique index and the partition key use different opclass.

I agree that this is an oversight, but it seems like your solution is
overcomplicated and probably still too forgiving.  Should we not just
insist that the index opfamily match the partition key opfamily?
It looks to me like that would reduce the code change to about like
this:

-               if (key->partattrs[i] == indexInfo->ii_IndexAttrNumbers[j])
+               if (key->partattrs[i] == indexInfo->ii_IndexAttrNumbers[j] &&
+                   key->partopfamily[i] == get_opclass_family(classObjectId[j]))

which is a lot more straightforward and provides a lot more certainty
that the index will act as the partition constraint demands.

This would reject, for example, a hash index associated with a btree-based
partition constraint, but I'm not sure we're losing anything much thereby.
(I do not think your patch is correct for the case where the opfamilies
belong to different AMs, anyway.)

I'm not really on board with adding a whole new test script for this,
either.

            regards, tom lane

> On Mar 26, 2020, at 01:00, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Guancheng Luo <prajnamort@gmail.com> writes:
>> I found that things could go wrong in some cases, when the unique index and the partition key use different opclass.
>
> I agree that this is an oversight, but it seems like your solution is
> overcomplicated and probably still too forgiving.  Should we not just
> insist that the index opfamily match the partition key opfamily?
> It looks to me like that would reduce the code change to about like
> this:
>
> -               if (key->partattrs[i] == indexInfo->ii_IndexAttrNumbers[j])
> +               if (key->partattrs[i] == indexInfo->ii_IndexAttrNumbers[j] &&
> +                   key->partopfamily[i] == get_opclass_family(classObjectId[j]))
>
> which is a lot more straightforward and provides a lot more certainty
> that the index will act as the partition constraint demands.
>
> This would reject, for example, a hash index associated with a btree-based
> partition constraint, but I'm not sure we're losing anything much thereby.
> (I do not think your patch is correct for the case where the opfamilies
> belong to different AMs, anyway.)

Since unique index cannot be using HASH, I think we only need to consider BTREE index here.

There is cases when a BTREE index associated with a HASH partition key, but I think we should allow them,
as long as their equality operators consider the same value as equal.
I’ve added some more test for this case.

> I'm not really on board with adding a whole new test script for this,
> either.

Indeed, I think `indexing.sql` might be more apporiate. I moved these tests in my new patch.



Best Regards,
Guancheng Luo

Guancheng Luo <prajnamort@gmail.com> writes:
> On Mar 26, 2020, at 01:00, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> This would reject, for example, a hash index associated with a btree-based
>> partition constraint, but I'm not sure we're losing anything much thereby.

> There is cases when a BTREE index associated with a HASH partition key, but I think we should allow them,
> as long as their equality operators consider the same value as equal.

Ah, yeah, I see we already have regression test cases that require that.

I pushed the patch with some cosmetic revisions.  I left out the
regression test case though; it seemed pretty expensive considering
that the code is already being exercised by existing cases.

Thanks for the report and patch!

            regards, tom lane