Horiguchi-san,
Thank you for updating the patch.
On 2018/04/16 16:17, Kyotaro HORIGUCHI wrote:
> the attached v6 patch differs only in gram.y since v5.
Patch fails to compile, because it adds get_partition_col_collation to
rel.h instead of partcache.h:
src/include/utils/rel.h: In function ‘get_partition_col_collation’:
src/include/utils/rel.h:594:12: error: dereferencing pointer to incomplete
type ‘struct PartitionKeyData’
PartitionKeyData definition was recently moved to partcache.h as part of
the big partition code reorganization.
> At Fri, 13 Apr 2018 18:55:30 +0900, Amit Langote wrote:
>> Looking at the gram.y changes in the latest patch, I think there needs to
>> be some explanatory comments about about the new productions -- u_expr,
>> b0_expr, and c0_expr.
>
> I think I did that. And refactord the rules.
>
> It was a bother that some rules used c_expr directly but I
> managed to replace all of them with a_expr by lowering precedence
> of some ordinary keywords (PASSING, BY, COLUMNS and ROW). c_expr
> is no loger used elsewhere so we can just remove columnref from
> c_expr. Finally [abc]0_expr was eliminated and we have only
> a_expr, b_expr, u_expr and c_expr. This seems simple enough.
>
> The relationship among the rules after this change is as follows.
>
> a_expr --+-- columnref
> +-- u_expr ---+-- c_expr -- (all old c_expr stuff except columnref)
> +-- (all old a_expr stuff)
>
> b_expr --+-- columnref
> +-- c_expr -- (ditto)
> +-- (all old b_expr stuff)
>
> On the way fixing this, I noticed that the precedence of some
> keywords (PRESERVE, STRIP_P) that was more than necessary and
> corrected it.
Thank you for simplifying gram.y changes. I think I was able to
understand them. Having said that, I think your proposed patch to gram.y
could be rewritten such that they do not *appear* to impact the syntax for
other features like XML, etc. For example, allowing a_expr in places
where previously only c_expr was allowed seems to me a dangerous, although
I don't have any examples to show for it.
It seems that we would like to use a_expr syntax but without columnref for
partition_bound expressions. No columnrefs because they cause conflicts
with unreserved keywords MINVALUE and MAXVALUE that are used in range
bound productions. I think that can be implemented with minimal changes
to expression syntax productions, as shown in the attached patch.
About the changes in transformPartitionBoundValue() to check for collation
conflict, I think that seems unnecessary. We're not going to use the
partition bound expression's collation anywhere, so trying to validate it
seems unnecessary. I wondered if we should issue a WARNING to warn the
user that COLLATE specified in a partition bound expression is ignored,
but not sure after seeing that we issue none when a user specifies
COLLATION in the expression for a column's DEFAULT value. We do still
store the COLLATION expression in pg_attrdef, but it doesn't seem to be
used anywhere.
Please find attached an updated version of your patch. I think we'll need
to make some documentation changes and think about a way to back-patch
this to PG10.
Thanks,
Amit
