|From:||Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>|
|To:||Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp>|
|Cc:||Pg Hackers <pgsql-hackers(at)postgresql(dot)org>|
|Subject:||Re: list partition constraint shape|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
(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.
> 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 =
+ ? get_array_type(key->parttypid)
+ : key->parttypid;
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;
+ arrexpr->element_typeid = key->parttypid;
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.
/* Generate the main expression, i.e., keyCol = ANY (arr) */
opexpr = make_partition_op_expr(key, 0, BTEqualStrategyNumber,
keyCol, (Expr *) elems);
/* If there are no partition values, we don't need an = ANY expr */
opexpr = NULL;
Other than that, the patch looks good to me.
|Next Message||Petr Jelinek||2018-01-25 12:18:34||Re: [PATCH] Logical decoding of TRUNCATE|
|Previous Message||Daniel Gustafsson||2018-01-25 11:40:28||Re: CONSTANT/NOT NULL/initializer properties for plpgsql record variables|