Horton

Thread

Commits

  1. Fix list partition constraints for partition keys of array type.

  1. list partition constraint shape

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-12-13T07:38:10Z

    Hi.

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.

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


Will add the patch to the next CF.

Thanks,
Amit

[1] https://www.postgresql.org/message-id/7677.1512743642%40sss.pgh.pa.us

  2. Re: list partition constraint shape

    Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> — 2018-01-23T11:13:52Z

    (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

  3. Re: list partition constraint shape

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-01-25T09:44:48Z

    Fujita-san,

Thanks for the review.

On 2018/01/23 20:13, Etsuro Fujita wrote:
> (2017/12/13 16:38), Amit Langote wrote:
>> 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.

Agreed, done that way.

So now, make_partition_op_expr() will generate an OR tree if the key is of
an array type, a ScalarArrayOpExpr if the IN-list contains more than one
value, and an OpExpr if the list contains just one value.

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

OK, added a test in create_table.sql.

Updated patch attached.

Thanks,
Amit

  4. Re: list partition constraint shape

    Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> — 2018-01-25T12:17:00Z

    (2018/01/25 18:44), Amit Langote wrote:
> On 2018/01/23 20:13, Etsuro Fujita wrote:
>> 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.
>
> Agreed, done that way.
>
> So now, make_partition_op_expr() will generate an OR tree if the key is of
> an array type, a ScalarArrayOpExpr if the IN-list contains more than one
> value, and an OpExpr if the list contains just one value.

Seems like a good idea.

>> * I think it'd be better to add a test case where multiple elements are
>> ORed together as a partition constraint.
>
> OK, added a test in create_table.sql.

Thanks.

> Updated patch attached.

Thanks for the updated patch!  Some minor comments:

+                   /*
+                    * Construct an ArrayExpr for the non-null partition
+                    * values
+                    */
+                   arrexpr = makeNode(ArrayExpr);
+                   arrexpr->array_typeid =
+                                   !type_is_array(key->parttypid[0])
+                                       ? get_array_type(key->parttypid[0])
+                                       : key->parttypid[0];

We test the type_is_array() above in this bit, so I don't think we need 
to test that again here.

+                   arrexpr->array_collid = key->parttypcoll[0];
+                   arrexpr->element_typeid = key->parttypid[0];

We can assume that keynum=0 here, so it would be okay to specify zero as 
the offset.  But ISTM that that makes code a bit less readable, so I'd 
vote for using keynum as the offset and maybe adding an assertion that 
keynum should be zero, somewhere in the PARTITION_STRATEGY_LIST block.

* Both comments in the following in get_qual_for_list needs to be 
updated, because the expression made there isn't necessarily = ANY anymore.

     if (elems)
     {
         /* Generate the main expression, i.e., keyCol = ANY (arr) */
         opexpr = make_partition_op_expr(key, 0, BTEqualStrategyNumber,
                                         keyCol, (Expr *) elems);
     }
     else
     {
         /* If there are no partition values, we don't need an = ANY expr */
         opexpr = NULL;
     }

Other than that, the patch looks good to me.

Best regards,
Etsuro Fujita

  5. Re: list partition constraint shape

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-01-26T01:15:49Z

    Fujita-san,

Thanks for the review.

On 2018/01/25 21:17, Etsuro Fujita wrote:
> Thanks for the updated patch!  Some minor comments:
> 
> +                   /*
> +                    * Construct an ArrayExpr for the non-null partition
> +                    * values
> +                    */
> +                   arrexpr = makeNode(ArrayExpr);
> +                   arrexpr->array_typeid =
> +                                   !type_is_array(key->parttypid[0])
> +                                       ? get_array_type(key->parttypid[0])
> +                                       : key->parttypid[0];
> 
> We test the type_is_array() above in this bit, so I don't think we need to
> test that again here.

Ah, you're right.  Fixed.

> +                   arrexpr->array_collid = key->parttypcoll[0];
> +                   arrexpr->element_typeid = key->parttypid[0];
> 
> We can assume that keynum=0 here, so it would be okay to specify zero as
> the offset.  But ISTM that that makes code a bit less readable, so I'd
> vote for using keynum as the offset and maybe adding an assertion that
> keynum should be zero, somewhere in the PARTITION_STRATEGY_LIST block.

Agreed, done.

> 
> * Both comments in the following in get_qual_for_list needs to be updated,
> because the expression made there isn't necessarily = ANY anymore.
> 
>     if (elems)
>     {
>         /* Generate the main expression, i.e., keyCol = ANY (arr) */
>         opexpr = make_partition_op_expr(key, 0, BTEqualStrategyNumber,
>                                         keyCol, (Expr *) elems);
>     }
>     else
>     {
>         /* If there are no partition values, we don't need an = ANY expr */
>         opexpr = NULL;
>     }

Fixed those.

Attached updated patch.  Thanks again.

Regards,
Amit

  6. Re: list partition constraint shape

    Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> — 2018-01-26T12:31:58Z

    (2018/01/26 10:15), Amit Langote wrote:
> On 2018/01/25 21:17, Etsuro Fujita wrote:
>> Some minor comments:
>>
>> +                   /*
>> +                    * Construct an ArrayExpr for the non-null partition
>> +                    * values
>> +                    */
>> +                   arrexpr = makeNode(ArrayExpr);
>> +                   arrexpr->array_typeid =
>> +                                   !type_is_array(key->parttypid[0])
>> +                                       ? get_array_type(key->parttypid[0])
>> +                                       : key->parttypid[0];
>>
>> We test the type_is_array() above in this bit, so I don't think we need to
>> test that again here.
>
> Ah, you're right.  Fixed.

Thanks.  I think the updated version is fine, but I think we can 
simplify the change in this part a bit further, so I modified your 
patch.  I also adjusted some comments in that change a little bit. 
Attached is a modified version of the patch.  What do you think about 
that?  Please let me know.  If that is okay, I'll mark this as Ready for 
Committer.

> Attached updated patch.  Thanks again.

Thanks for updating the patch!

Best regards,
Etsuro Fujita

  7. Re: list partition constraint shape

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-01-29T00:50:50Z

    Fujita-san,

On 2018/01/26 21:31, Etsuro Fujita wrote:
> (2018/01/26 10:15), Amit Langote wrote:
>> On 2018/01/25 21:17, Etsuro Fujita wrote:
>>> Some minor comments:
>>>
>>> +                   /*
>>> +                    * Construct an ArrayExpr for the non-null partition
>>> +                    * values
>>> +                    */
>>> +                   arrexpr = makeNode(ArrayExpr);
>>> +                   arrexpr->array_typeid =
>>> +                                   !type_is_array(key->parttypid[0])
>>> +                                       ?
>>> get_array_type(key->parttypid[0])
>>> +                                       : key->parttypid[0];
>>>
>>> We test the type_is_array() above in this bit, so I don't think we need to
>>> test that again here.
>>
>> Ah, you're right.  Fixed.
> 
> Thanks.  I think the updated version is fine, but I think we can simplify
> the change in this part a bit further, so I modified your patch.  I also
> adjusted some comments in that change a little bit. Attached is a modified
> version of the patch.  What do you think about that?  Please let me know. 
> If that is okay, I'll mark this as Ready for Committer.

That looks good, thanks.

Regards,
Amit

  8. Re: list partition constraint shape

    Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> — 2018-01-29T06:15:14Z

    (2018/01/29 9:50), Amit Langote wrote:
> On 2018/01/26 21:31, Etsuro Fujita wrote:
>> Attached is a modified
>> version of the patch.  What do you think about that?  Please let me know.
>> If that is okay, I'll mark this as Ready for Committer.
>
> That looks good, thanks.

Cool!  One thing I noticed to revise the patch a bit further is to add 
an assertion to make_partition_op_expr that the number of the right-hand 
inputs to generate an operator expression from for list-partitioning 
should be >=1; in that case we call that function if there is at least 
one non-null partition value as the right-hand input, so that should be 
asserted successfully there.  I think the assertion is a good thing, so 
I modified the patch.  Updated patch attached.  Because I made no 
changes other than that, I'll make this as Ready for Committer.

Best regards,
Etsuro Fujita

  9. Re: list partition constraint shape

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-01-29T06:22:20Z

    Fujita-san,

On 2018/01/29 15:15, Etsuro Fujita wrote:
> (2018/01/29 9:50), Amit Langote wrote:
>> On 2018/01/26 21:31, Etsuro Fujita wrote:
>>> Attached is a modified
>>> version of the patch.  What do you think about that?  Please let me know.
>>> If that is okay, I'll mark this as Ready for Committer.
>>
>> That looks good, thanks.
> 
> Cool!  One thing I noticed to revise the patch a bit further is to add an
> assertion to make_partition_op_expr that the number of the right-hand
> inputs to generate an operator expression from for list-partitioning
> should be >=1; in that case we call that function if there is at least one
> non-null partition value as the right-hand input, so that should be
> asserted successfully there.  I think the assertion is a good thing, so I
> modified the patch.

Ah, good call.

>  Updated patch attached.  Because I made no changes
> other than that, I'll make this as Ready for Committer.
Thanks a lot for reviewing.

Regards,
Amit

  10. Re: list partition constraint shape

    Robert Haas <robertmhaas@gmail.com> — 2018-01-31T21:08:55Z

    On Mon, Jan 29, 2018 at 1:22 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
>>  Updated patch attached.  Because I made no changes
>> other than that, I'll make this as Ready for Committer.
> Thanks a lot for reviewing.

Committed and back-patched to v10.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  11. Re: list partition constraint shape

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-02-01T01:05:24Z

    On 2018/02/01 6:08, Robert Haas wrote:
> On Mon, Jan 29, 2018 at 1:22 AM, Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>>>  Updated patch attached.  Because I made no changes
>>> other than that, I'll make this as Ready for Committer.
>> Thanks a lot for reviewing.
> 
> Committed and back-patched to v10.

Thank you.

Regards,
Amit