On 22 February 2018 at 22:31, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> 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].
I've made that 0001 in the series
> * 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 wasn't aware of that gcc flag. I was also surprised to see a clean
compile from master with it enabled. This area has been changed a bit
from the last patch, but the remaining switch now has a default:
return false;
> * I see that you moved PartitionClauseInfo's definition from partprune.c
> to partition.h. Isn't it better to move it to partprune.h?
Moved. I'd done it the other way to try to reduce the number of
planner headers included in the executor, but will defer to your
better judgement, as I see you're busy working on improving this area
in another patch set.
I've attached an updated set of patches.
I hope this also addresses Rajkumar reported crash. I ended up making
some changes to how the Param values are determined by reusing more of
the existing executor code rather than duplicating it in
partkey_datum_from_expr. I really could use a sanity check on my
changes to that function now, especially the cross type portion.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services