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