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