Re: speeding up planning with partitions

Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
> Here are some comments on v38.

Thanks for looking it over!  I'll just reply to points worth discussing:

> -            Assert(rte->rtekind == RTE_RELATION ||
> -                   rte->rtekind == RTE_SUBQUERY);
> -            add_appendrel_other_rels(root, rel, rti);
> +            if (rte->rtekind == RTE_RELATION)
> +                expand_inherited_rtentry(root, rel, rte, rti);
> +            else
> +                expand_appendrel_subquery(root, rel, rte, rti);

> Wouldn't it be a good idea to keep the Assert?

There's an Assert in expand_appendrel_subquery that what it got is an
RTE_SUBQUERY, so I thought the one at the call site was redundant.
I suppose another way to do this would be like

           if (rte->rtekind == RTE_RELATION)
               expand_inherited_rtentry(root, rel, rte, rti);
           else if (rte->rtekind == RTE_SUBQUERY)
               expand_appendrel_subquery(root, rel, rte, rti);
           else
               Assert(false);

Not sure if that's better or not.  Or we could go back to the
design of just having one function and letting it dispatch the
case it doesn't want to the other function --- though I think
I'd make expand_inherited_rtentry be the primary function,
rather than the other way around as you had it in v37.

> +    forboth(lc, old_child_rtis, lc2, new_child_rtis)
> +    {
> +        int         old_child_rti = lfirst_int(lc);
> +        int         new_child_rti = lfirst_int(lc2);
> +
> +        if (old_child_rti == new_child_rti)
> +            continue;           /* nothing to do */
> +
> +        Assert(old_child_rti > new_child_rti);
> +
> +        ChangeVarNodes((Node *) child_appinfos,
> +                       old_child_rti, new_child_rti, 0);
> +    }

> This seems necessary?  RTEs of children of the target table should be in
> the same position in the final_rtable as they are in the select_rtable.

Well, that's what I'm not very convinced of.  I have observed that
the regression tests don't reach this ChangeVarNodes call, but
I think that might just be lack of test cases rather than a proof
that it's impossible.  The question is whether it'd ever be possible
for the update/delete target to not be the first "inh" table that
gets expanded.  Since that expansion is done in RTE order, it
reduces to "is the target always before any other RTE entries
that could need inheritance expansion?"  Certainly that would typically
be true, but I don't feel very comfortable about assuming that it
must be true, when you start thinking about things like updatable
views, rules, WITH queries, and so on.

It might be worth trying to devise a test case that does reach this
code.  If we could convince ourselves that it's really impossible,
I'd be willing to drop it in favor of putting a test-and-elog check
in the earlier loop that the RTI pairs are all equal.  But I'm not
willing to do it without more investigation.

> +        /* XXX wrong? */
> +        parentrte->inh = false;

> About the XXX: I think resetting inh flag is unnecessary, so we should
> just remove the line.

Possibly.  I hadn't had time to follow up the XXX annotation.

> If we do that, we can also get rid of the following
> code in set_rel_size():

>                 else if (rte->relkind == RELKIND_PARTITIONED_TABLE)
>                 {
>                     /*
>                      * A partitioned table without any partitions is marked as
>                      * a dummy rel.
>                      */
>                     set_dummy_rel_pathlist(rel);
>                 }

Not following?  Surely we need to mark the childless parent as dummy at
some point, and that seems like as good a place as any.

> Finally, it's not in the patch, but how about visiting
> get_relation_constraints() for revising this block of code:

That seems like probably an independent patch --- do you want to write it?

			regards, tom lane