Re: using expression syntax for partition bounds

From: Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp>
To: Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
Cc: pgsql-hackers(at)postgresql(dot)org, tgl(at)sss(dot)pgh(dot)pa(dot)us
Subject: Re: using expression syntax for partition bounds
Date: 2018-06-26 01:41:11
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers


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
> | 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:

>> /*
>> * 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)


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


Attachment Content-Type Size
v4-0001-Allow-generalized-expression-syntax-for-partition.patch text/plain 29.0 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Kyotaro HORIGUCHI 2018-06-26 02:41:22 Re: PATCH: backtraces for error messages
Previous Message David G. Johnston 2018-06-26 01:33:40 Re: automatic restore point