(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
