Re: using expression syntax for partition bounds

Horiguchi-san,

Thanks a lot for the review and sorry it took me a while to reply.
Thought I'd refresh the patch as it's in the July CF.

On 2018/04/24 18:08, Kyotaro HORIGUCHI wrote:
> Thanks. I have almost missed this.
> 
> At Mon, 23 Apr 2018 11:44:14 +0900, Amit Langote wrote:
>> On 2018/04/23 11:37, Amit Langote wrote:
>>> I tried to update the patch to do things that way.  I'm going to create a
>>> new entry in the next CF titled "generalized expression syntax for
>>> partition bounds" and add the patch there.
>>
>> Tweaked the commit message to credit all the authors.
> 
> +      any variable-free expression (subqueries, window functions, aggregate
> +      functions, and set-returning functions are not allowed).  The data type
> +      of the partition bound expression must match the data type of the
> +      corresponding partition key column.
> 
> Parititioning value is slimiar to column default expression in
> the sense that it appeas within a DDL. The description for
> DEFAULT is:
> 
> |  The value is any variable-free expression (subqueries and
> |  cross-references to other columns in the current table are not
> |  allowed)
> 
> It actually refuses aggregates, window functions and SRFs but it
> is not mentioned.  Whichever we choose, they ought to have the
> similar description.

Actually, I referenced the default value expression syntax a lot when
working on this issue, so agree that there's a close resemblance.

Maybe, we should fix the description for default expression to say that it
can't contain subqueries, cross-references to other columns in the table,
aggregate expressions, window functions, and set-returning functions.  I
also sent a patch separately:

https://www.postgresql.org/message-id/2059e8f2-e6e6-7a9f-0de8-11eed291e641@lab.ntt.co.jp

>>   /*
>>    * Strip any top-level COLLATE clause, as they're inconsequential.
>>    * XXX - Should we add a WARNING here?
>>    */
>>   while (IsA(value, CollateExpr))
>>       value = (Node *) ((CollateExpr *) value)->arg;
> 
> Ok, I'll follow Tom's suggestion but collate is necessarilly
> appears in this shape. For example ('a' collate "de_DE") || 'b')
> has the collate node under the top ExprOp and we get a complaint
> like following with it.
> 
>> ERROR:  unrecognized node type: 123
> 
> 123 is T_CollateExpr. The expression "value" (mmm..) reuires
> eval_const_expressions to eliminate CollateExprs.  It requires
> assign_expr_collations to retrieve value's collation but we don't
> do that since we ignore collation this in this case.

Ah yes, it seems better to call eval_const_expressions as a first step to
get rid of CollateExpr's, followed by evaluate_expr if the first step
didn't return a Const node.

> The following results in a strange-looking error.
> 
>> =# create table pt1 partition of p for values in (a);
>> ERROR:  column "a" does not exist
>> LINE 1: create table pt1 partition of p for values in (a);
> 
> The p/pt1 actually have a column a.
> 
> The following results in similar error and it is wrong, too.
> 
>> =# create table pr1 partition of pr for values from (a + 1) to (a + 2);
>> ERROR: column "a" does not exist
>> LINE 1: create table pr1 partition of pr for values from (a + 1) to ...

This one is better fixed by initializing ParseState properly as
demonstrated in your v3-2 patch.

> Being similar but a bit different, the following command leads to
> a SEGV since it creates PARTITION_RANGE_DATUM_VALUE with value =
> NULL. Even if it leaves the original node, validateInfiniteBounds
> rejects it and aborts.
> 
>> =# create table pr1 partition of pr for values from (hoge) to (hige);
> (crash)

Oops.

> I fixed this using pre-columnref hook in the attached v3. This
> behavles for columnrefs differently than DEFAULT.
> 
> The v3-2 does the same thing with DEFAULT. The behevior differs
> whether the column exists or not.
> 
>> ERROR:  cannot use column referencees in bound value
>> ERROR:  column "b" does not exist
> 
> I personally think that such similarity between DEFALUT and
> partition bound (v3-2) is not required. But inserting the hook
> (v3) doesn't look good for me.

Actually, I too like the solution of v3-2 better, instead of using the
hook, so I adopted it in the updated patch.

I also changed how range bounds are processed in transformPartitionBound,
moving some code into newly added transformPartitionRangeBounds to reduce
code duplication.

Updated patch attached.

Thanks,
Amit