Re: speeding up planning with partitions

Here are some comments on v38.

On 2019/03/29 12:44, Amit Langote wrote:
> Thanks again for the new patch.  I'm reading it now and will send comments
> later today if I find something.

-            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?

+     * It's possible that the RTIs we just assigned for the child rels in the
+     * final rtable are different from where they were in the SELECT query.

In the 2nd sentence, maybe you meant "...from what they were"

+    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.
It seems that they can be added to parse->rtable simply as:

  orig_rtable_len = list_length(parse->rtable);
  parse->rtable = list_concat(parse->rtable,
                              list_copy_tail(select_rtable,
                                             orig_rtable_len));

That is, after the block of code that plans the query as SELECT.

+         * about the childrens' reltargets, they'll be made later

Should it be children's?

+    /*
+     * If the partitioned table has no partitions, treat this as the
+     * non-inheritance case.
+     */
+    if (partdesc->nparts == 0)
+    {
+        /* XXX wrong? */
+        parentrte->inh = false;
+        return;
+    }

About the XXX: I think resetting inh flag is unnecessary, so we should
just remove the line.  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);
                }


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

/*
 * Append partition predicates, if any.
 *
 * For selects, partition pruning uses the parent table's partition bound
 * descriptor, instead of constraint exclusion which is driven by the
 * individual partition's partition constraint.
 */
if (enable_partition_pruning && root->parse->commandType != CMD_SELECT)
{
    List       *pcqual = RelationGetPartitionQual(relation);

    if (pcqual)
    {
        /*
         * Run the partition quals through const-simplification similar to
         * check constraints.  We skip canonicalize_qual, though, because
         * partition quals should be in canonical form already; also,
         * since the qual is in implicit-AND format, we'd have to
         * explicitly convert it to explicit-AND format and back again.
         */
        pcqual = (List *) eval_const_expressions(root, (Node *) pcqual);

        /* Fix Vars to have the desired varno */
        if (varno != 1)
            ChangeVarNodes((Node *) pcqual, 1, varno, 0);

        result = list_concat(result, pcqual);
    }
}

We will no longer need to load the partition constraints for "other rel"
partitions, not even for UPDATE and DELETE queries.  Now, we won't load
them with the patch applied, because we're cheating by first planning the
query as SELECT, so that's not an issue.  But we should change the
condition here to check if the input relation is a "baserel", in which
case, this should still load the partition constraint so that constraint
exclusion can use it when running with constraint_exclusion = on.  In
fact, I recently reported [1] on -hackers that we don't load the partition
constraint even if the partition is being accessed directly as a bug
introduced in PG 11.

Thanks,
Amit

[1]
https://www.postgresql.org/message-id/9813f079-f16b-61c8-9ab7-4363cab28d80%40lab.ntt.co.jp