Re: using expression syntax for partition bounds

Thanks for the review.

On 2019/01/15 22:24, Peter Eisentraut wrote:
> On 15/01/2019 07:31, Amit Langote wrote:
>>> 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.
>
> I was more concerned about the term "bound", which it is not range
> partitioning. But I can't think of a better term.
>
> I wouldn't change expr to value as you have done between v7 and v8,
> since the point of this patch is to make expressions valid where
> previously only values were.

OK, will change it back to partition_bound_expr. Removing "bound" from it
makes the term ambiguous?

>>> 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
>
> OK, if we are going with that approach, then it needs to be documented
> and there should be test cases.

How about the following note in the documentation:

+ Although volatile expressions such as
+ <literal><function>CURRENT_TIMESTAMP</function></literal> can be used
+ for this, be careful when using them, because
+ <productname>PostgreSQL</productname> will evaluate them only once
+ when adding the partition.

Sorry but I'm not sure how or what I would test about this. Maybe, just
add a test in create_table.sql/alter_table.sql that shows that using
volatile expression doesn't cause an error?

>>> 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.
>
>> 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.
>
> I think that needs more refinement. In v8, the following errors
>
> CREATE TABLE t2 ( a name COLLATE "POSIX" ) PARTITION BY RANGE (a);
> CREATE TABLE t2a PARTITION OF t2 FOR VALUES FROM (name 'foo') TO (name
> 'xyz');
> ERROR: collation of partition bound value for column "a" does not match
> partition key collation "POSIX"
>
> The problem here is that the "name" type has a collation that is neither
> the one of the column nor the default collation. We can allow that.

So, should the "name" type's collation should simply be discarded in favor
of "POSIX" that's being used for partitioning?

> What we don't want is someone writing an explicit COLLATE clause. I
> think we just need to check that there is no top-level COLLATE clause.
> This would then sort of match the logic parse_collate.c for combining
> collations.

Maybe I'm missing something, but isn't it OK to allow the COLLATE clause
as long as it specifies the matching collation as the parent? So:

CREATE TABLE t2b PARTITION OF t2 FOR VALUES FROM (name 'bar' collate "C")
TO (name 'foo');
ERROR: collation of partition bound value for column "a" does not match
partition key collation "POSIX"

-- either of the following is ok

CREATE TABLE t2b PARTITION OF t2 FOR VALUES FROM (name 'bar' collate
"POSIX") TO (name 'foo');

CREATE TABLE t2b PARTITION OF t2 FOR VALUES FROM (name 'bar') TO (name 'foo');

Thanks,
Amit