Re: [Sender Address Forgery]Re: path toward faster partition pruning

From: Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp>
To: David Rowley <david(dot)rowley(at)2ndquadrant(dot)com>
Cc: Dilip Kumar <dilipbalaut(at)gmail(dot)com>, Rajkumar Raghuwanshi <rajkumar(dot)raghuwanshi(at)enterprisedb(dot)com>, Beena Emerson <memissemerson(at)gmail(dot)com>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [Sender Address Forgery]Re: path toward faster partition pruning
Date: 2017-11-08 05:23:18
Message-ID: 4a1afba7-d890-27da-e978-aaf8d3fb7711@lab.ntt.co.jp
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi David.

Thanks for the review.

(..also looking at the comments you sent earlier today.)

On 2017/11/07 11:14, David Rowley wrote:
> On 7 November 2017 at 01:52, David Rowley <david(dot)rowley(at)2ndquadrant(dot)com> wrote:
>> Thanks. I'll look over it all again starting my Tuesday morning. (UTC+13)
>
> I have a little more review to share:
>
> 1. Missing "in" in comment. Should be "mentioned in"
>
> * get_append_rel_partitions
> * Return the list of partitions of rel that pass the clauses mentioned
> * rel->baserestrictinfo
>
> 2. Variable should be declared in inner scope with the following fragment:
>
> void
> set_basic_child_rel_properties(PlannerInfo *root,
> RelOptInfo *rel,
> RelOptInfo *childrel,
> AppendRelInfo *appinfo)
> {
> AttrNumber attno;
>
> if (rel->part_scheme)
> {
>
> which makes the code the same as where you moved it from.

It seems like you included the above changes in your attached C file,
which I will incorporate into my repository.

> 3. Normally lfirst() is assigned to a variable at the start of a
> foreach() loop. You have code which does not follow this.
>
> foreach(lc, clauses)
> {
> Expr *clause;
> int i;
>
> if (IsA(lfirst(lc), RestrictInfo))
> {
> RestrictInfo *rinfo = lfirst(lc);
>
> You could assign this to a Node * since the type is unknown to you at
> the start of the loop.

Will make the suggested changes to match_clauses_to_partkey().

> 4.
> /*
> * Useless if what we're thinking of as a constant is actually
> * a Var coming from this relation.
> */
> if (bms_is_member(rel->relid, constrelids))
> continue;
>
> should this be moved to just above the op_strict() test? This one seems cheaper.

Agreed, will do. Also makes sense to move the PartCollMatchesExprColl()
test together.

> 5. Typo "paritions": /* No clauses to prune paritions, so scan all
> partitions. */
>
> But thinking about it more the comment should something more along the
> lines of /* No useful clauses for partition pruning. Scan all
> partitions. */

You fixed it. :)

> The key difference is that there might be clauses, just without Consts.
>
> Actually, the more I look at get_append_rel_partitions() I think it
> would be better if you re-shaped that if/else if test so that it only
> performs the loop over the partindexes if it's been set.
>
> I ended up with the attached version of the function after moving
> things around a little bit.

Thanks a lot for that. Looks much better now.

> I'm still reviewing but thought I'd share this part so far.

As mentioned at the top, I'm looking at your latest comments and they all
seem to be good points to me, so will address those in the next version.

Thanks,
Amit

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Thomas Munro 2017-11-08 05:33:09 Re: UPDATE of partition key
Previous Message Amit Khandekar 2017-11-08 04:57:34 Re: UPDATE of partition key