Re: list partition constraint shape

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
Date: 2018-01-25 12:17:00
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

(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 =
+ !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);
/* 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

In response to


Browse pgsql-hackers by date

  From Date Subject
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