Re: using expression syntax for partition bounds - Mailing list pgsql-hackers

Thanks for the review and sorry it took me a while to reply.

On 2019/01/02 22:58, Peter Eisentraut wrote:
> On 26/11/2018 05:58, Amit Langote wrote:
>> On 2018/11/09 14:38, Amit Langote wrote:
>>> Rebased due to change in addRangeTableEntryForRelation's API.
>>
>> Rebased again due to changes in psql's describe output for partitioned tables.
> 
> Review:
> 
> Is "partition bound" the right term?  For list partitioning, it's not
> really a bound.  Maybe it's OK.

Hmm, maybe "partition bound value"?  Just want to stress that the
expression specifies "bounding" value of a partition.

> Keep the ordering of EXPR_KIND_PARTITION_BOUND in the various switch
> statements consistent.

OK, fixed.

> I don't see any treatment of non-immutable expressions.  There is a test
> case with a non-immutable cast that was removed, but that's all.  There
> was considerable discussion earlier in the thread on whether
> non-immutable expressions should be allowed.  I'm not sure what the
> resolution was, but in any case there should be documentation and tests
> of the outcome.

The partition bound expression is evaluated only once, that is, when the
partition creation command is executed, and what gets stored in the
catalog is a Const expression that contains the value that was computed,
not the original non-immutable expression.  So, we don't need to do
anything special in terms of code to handle users possibly specifying a
non-immutable expression as partition bound.  Tom said something in that
thread which seems to support such a position:

https://www.postgresql.org/message-id/22534.1523374457%40sss.pgh.pa.us

Part of the test case that was removed is the error that used to be emitted:

 CREATE TABLE moneyp (
     a money
 ) PARTITION BY LIST (a);
 CREATE TABLE moneyp_10 PARTITION OF moneyp FOR VALUES IN (10);
-ERROR:  specified value cannot be cast to type money for column "a"
-LINE 1: ...EATE TABLE moneyp_10 PARTITION OF moneyp FOR VALUES IN (10);
-                                                                   ^
-DETAIL:  The cast requires a non-immutable conversion.
-HINT:  Try putting the literal value in single quotes.
-CREATE TABLE moneyp_10 PARTITION OF moneyp FOR VALUES IN ('10');

which is no longer emitted, because the patched
transformPartitionBoundValue evaluates the expression, instead of emitting
error because the expression hasn't become a Const even after applying
eval_const_expressions().

Do we need to mention any aspect of how this now works in the user-facing
documentation?

> The collation handling might need another look.  The following is
> allowed without any diagnostic:
> 
> CREATE TABLE t2 (
>     a text COLLATE "POSIX"
> ) PARTITION BY RANGE (a);
> CREATE TABLE t2a PARTITION OF t2 FOR VALUES FROM ('foo' COLLATE "C") TO
> ('xyz');
> 
> I think the correct behavior would be that an explicit collation in the
> partition bound expression is an error.

I tend to agree with that.  What happens is the partition bound expression
is assigned the collation of the parent's partition key:

+        partcollation = get_partition_col_collation(key, 0);

+            value = transformPartitionBoundValue(pstate, expr,
+                                                 colname, coltype, coltypmod,
+                                                 partcollation);

But that's essentially ignoring the collation specified by the user for
the partition bound value without providing any information about that to
the user.  I've fixed that by explicitly checking if the collate clause in
the partition bound value expression contains the same collation as the
parent partition key collation and give an error otherwise.

Updated patch attached.

Thanks,
Amit