Re: list partition constraint shape

Fujita-san,

Thanks for the review.

On 2018/01/25 21:17, Etsuro Fujita wrote:
> 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.

Ah, you're right.  Fixed.

> +                   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.

Agreed, done.

> 
> * 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;
>     }

Fixed those.

Attached updated patch.  Thanks again.

Regards,
Amit