Re: list partition constraint shape

(2017/12/13 16:38), Amit Langote wrote:
> I recently posted to the list about a couple of problems I saw when using
> array type column as the partition key. One of them was that the internal
> partition constraint expression that we generate for list partitions is of
> a form that the backend would reject if the partition key column is an
> array instead of a scalar. See for example:
>
> create table p (a int[]) partition by list (a);
> create table p1 partition of p for values in ('{1}');
> create table p2 partition of p for values in ('{2, 3}', '{4, 5}');
>
> insert into p values ('{1}');
> INSERT 0 1
> insert into p values ('{2, 3}'), ('{4, 5}');
> INSERT 0 2
>
> \d+ p1
> ...
> Partition of: p FOR VALUES IN ('{1}')
> Partition constraint: ((a IS NOT NULL) AND ((a)::anyarray
> OPERATOR(pg_catalog.=) ANY (ARRAY['{1}'::integer[]])))
>
> \d+ p2
> ...
> Partition of: p FOR VALUES IN ('{2,3}', '{4,5}')
> Partition constraint: ((a IS NOT NULL) AND ((a)::anyarray
> OPERATOR(pg_catalog.=) ANY (ARRAY['{2,3}'::integer[], '{4,5}'::integer[]])))
>
> Try copy-pasting the p1's constraint into SQL:
>
> In a select query:
>
> select tableoid::regclass, (a)::anyarray OPERATOR(pg_catalog.=) ANY
> (ARRAY['{1}'::integer[]]) from p;
> ERROR: operator does not exist: integer[] pg_catalog.= integer
> LINE 1: select tableoid::regclass, (a)::anyarray OPERATOR(pg_catalog...
> ^
> HINT: No operator matches the given name and argument types. You might
> need to add explicit type casts.
>
> Or use in a check constraint:
>
> alter table p1 add constraint check_a check ((a)::anyarray
> OPERATOR(pg_catalog.=) ANY (ARRAY['{1}'::integer[]]));
> ERROR: operator does not exist: integer[] pg_catalog.= integer
> HINT: No operator matches the given name and argument types. You might
> need to add explicit type casts.
>
> That's because, as Tom pointed out [1], ANY/ALL expect the LHS to be a
> scalar, whereas in this case a is an int[]. So, the partitioning code is
> internally generating an expression that would not get through the parser.
> I think it's better that we fix that.

+1

> Attached patch is an attempt at that. With the patch, instead of
> internally generating an ANY/ALL expression, generate an OR expression
> instead. So:
>
> \d+ p1
> ...
> Partition of: p FOR VALUES IN ('{1}')
> Partition constraint: ((a IS NOT NULL) AND ((a)::anyarray
> OPERATOR(pg_catalog.=) '{1}'::integer[]))
>
> \d+ p2
> ...
> Partition of: p FOR VALUES IN ('{2,3}', '{4,5}')
> Partition constraint: ((a IS NOT NULL) AND (((a)::anyarray
> OPERATOR(pg_catalog.=) '{2,3}'::integer[]) OR ((a)::anyarray
> OPERATOR(pg_catalog.=) '{4,5}'::integer[])))
>
> The expressions above get through the parser just fine:
>
> select tableoid::regclass, (a)::anyarray OPERATOR(pg_catalog.=)
> '{1}'::integer[] from p;
> tableoid | ?column?
> |---------+----------
> p1 | t
> p2 | f
> p2 | f
> (3 rows)
>
> alter table p1 add constraint check_a check ((a)::anyarray
> OPERATOR(pg_catalog.=) '{1}'::integer[]);
> ALTER TABLE
>
> \d+ p1
> ...
> Check constraints:
> "check_a" CHECK (a = '{1}'::integer[])

Thanks for the patch! Here are review comments that I have for now:

* I think it's a good idea to generate an OR expression tree for the
case where the type of the partitioning key is an array, but I'm not
sure we should handle other cases the same way because partition
constraints represented by OR-expression trees would not be efficiently
processed by the executor, compared to ScalarArrayOpExpr, when the
number of elements that are ORed together is large. So what about
generating the OR expression tree only if the partitioning-key's type is
an array, instead? That would be more consistent with the handling of
IN-list check constraints in eg, CREATE/ALTER TABLE, which I think is a
good thing.

* I think it'd be better to add a test case where multiple elements are
ORed together as a partition constraint.

Best regards,
Etsuro Fujita