Re: list partition constraint shape

(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