Hi David.
On 2018/02/21 18:06, David Rowley wrote:
> Other things I don't particularly like about the current patch are how
> I had to construct the partition key expressions in
> set_valid_runtime_subplans_recurse(). This pretty much uses code
> borrowed from set_baserel_partition_key_exprs(). One way around that
> would be to change the partprune.c code to deal with the
> partkey->partattrs and consume an expression from the list on attnum =
> 0. I didn't do that as I want to minimise how much I touch Amit's
> patch before it gets committed as doing so would likely just cause
> more headaches for me keeping this merged with his work. Another
> option to resolve this would be to put the partition key expressions
> into the PartitionPruneInfo.
Another way could be to refactor the code you've borrowed from
set_baserel_partition_key_exprs() into its own function that is exported
by some optimizer header.
I removed PartitionKey/Relation from signatures of various functions of my
patch to avoid having to re-heap_open() the relation per [1].
> I've attached v11 of the patch.
Some comments:
* I noticed that the patch adds a function to bitmapset.c which you might
want to extract into its own patch, like your patch to add bms_add_range()
that got committed as 84940644d [2].
* Maybe it's better to add `default: break;` in the two switch's
you've added in partkey_datum_from_expr()
partprune.c: In function ‘partkey_datum_from_expr’:
partprune.c:1555:2: warning: enumeration value ‘T_Invalid’ not handled in
switch [-Wswitch]
switch (nodeTag(expr))
partprune.c:1562:4: warning: enumeration value ‘PARAM_SUBLINK’ not handled
in switch [-Wswitch]
switch (((Param *) expr)->paramkind)
* I see that you moved PartitionClauseInfo's definition from partprune.c
to partition.h. Isn't it better to move it to partprune.h?
Thanks,
Amit
[1]
https://www.postgresql.org/message-id/CA%2BTgmoabi-29Vs8H0xkjtYB%3DcU%2BGVCrNwPz7okpa3KsoLmdEUQ%40mail.gmail.com
[2] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=84940644d