Horton

Re: [HACKERS] Runtime Partition Pruning

David Rowley <david.rowley@2ndquadrant.com>

View thread View on postgr.es
From: David Rowley <david.rowley@2ndquadrant.com>
To: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
Cc: Rajkumar Raghuwanshi <rajkumar.raghuwanshi@enterprisedb.com>, Beena Emerson <memissemerson@gmail.com>, Alvaro Herrera <alvherre@alvh.no-ip.org>, Robert Haas <robertmhaas@gmail.com>, amul sul <sulamul@gmail.com>, "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>, Dilip Kumar <dilipbalaut@gmail.com>
Date: 2018-02-23T11:34:19Z
Lists: pgsql-hackers

Attachments

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

Commits

  1. Initialize ExprStates once in run-time partition pruning

  2. Add bms_prev_member function

  3. Support partition pruning at execution time

  4. Document partprune.c a little better

  5. Fix crash in pg_replication_slot_advance

  6. Add parallel-aware hash joins.

  7. Fix typo.

  8. Allow --with-bonjour to work with non-macOS implementations of Bonjour.

  9. Mention need for --no-inc-recursive in rsync command