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.row...@2ndquadrant.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
