RE: speeding up planning with partitions

Hi Amit,

On Tue, Nov 20, 2018 at 10:24 PM, Amit Langote wrote:
> Attached v8 patches.

Thanks for the patch. I took a look 0003, 0005, 0006 of v8 patch.

1.
0003: line 267-268
+ * Child relation may have be marked dummy if build_append_child_rel
+ * found self-contradictory quals.

/s/have be marked/have been marked/

2.
0003: around line 1077
In append.c(or prepunion.c)
228 * Check that there's at least one descendant, else treat as no-child
229 * case. This could happen despite above has_subclass() check, if table
230 * once had a child but no longer does.

has_subclass() check is moved to subquery_planner from above this code,
so the comments need to be modified like below.

s/above has_subclass() check/has_subclass() check in subquery_planner/

3.
0003: line 1241-1244
0006: line ?

In comments of expand_partitioned_rtentry:
+ * Note: even though only the unpruned partitions will be added to the
+ * resulting plan, this still locks *all* partitions via find_all_inheritors
+ * in order to avoid partitions being locked in a different order than other
+ * places in the backend that may lock partitions.

This comments is no more needed if 0006 patch is applied because
find_all_inheritors is removed in the 0006 patch.

4.
0003: line 1541-1544

+ * Add the RelOptInfo. Even though we may not really scan this relation
+ * for reasons such as contradictory quals, we still need need to create
+ * one, because for every RTE in the query's range table, there must be an
+ * accompanying RelOptInfo.

s/need need/need/

5.
0003: line 1620-1621

+ * After creating the RelOptInfo for the given child RT index, it goes on to
+ * initialize some of its fields base on the parent RelOptInfo.

s/fields base on/fields based on/

6.
parsenodes.h
906 * inh is true for relation references that should be expanded to include
907 * inheritance children, if the rel has any. This *must* be false for
908 * RTEs other than RTE_RELATION entries.

I think inh can become true now even if RTEKind equals RTE_SUBQUERY, so latter
sentence need to be modified.

7.
0005: line 109-115
+ /*
+ * If partition is excluded by constraints, remove it from
+ * live_parts, too.
+ */
+ if (IS_DUMMY_REL(childrel))
+ parentrel->live_parts = bms_del_member(parentrel->live_parts, i);
+

When I read this comment, I imagined that relation_excluded_by_constraints()
would be called before this code. childrel is marked dummy if
build_append_child_rel found self-contradictory quals, so comments can be
modified more correctly like another comments in your patch as below.

In 0003: line 267-271
+ * Child relation may have be marked dummy if build_append_child_rel
+ * found self-contradictory quals.
+ */
+ if (IS_DUMMY_REL(childrel))
+ continue;

8.
0003: line 705-711
+ * Query planning may have added some columns to the top-level tlist,
+ * which happens when there are row marks applied to inheritance
+ * parent relations (additional junk columns needed for applying row
+ * marks are added after expanding inheritance.)
+ */
+ if (list_length(tlist) < list_length(root->processed_tlist))
+ tlist = root->processed_tlist;

In grouping_planner():

if (planned_rel == NULL)
{
...
root->processed_tlist = tlist;
}
else
tlist = root->processed_tlist;
...
if (current_rel == NULL)
current_rel = query_planner(root, tlist,
standard_qp_callback, &qp_extra);
...
/*
* Query planning may have added some columns to the top-level tlist,
* which happens when there are row marks applied to inheritance
* parent relations (additional junk columns needed for applying row
* marks are added after expanding inheritance.)
*/
if (list_length(tlist) < list_length(root->processed_tlist))
tlist = root->processed_tlist;

Are there any case tlist points to an address different from
root->processed_tlist after calling query_planner? Junk columns are possibly
added to root->processed_tlist after expanding inheritance, but that adding
process don't change the root->processed_tlist's pointer address.
I checked "Assert(tlist == root->processed_tlist)" after calling query_planner
passes "make check".

9.
0003: line 1722-1763
In build_append_child_rel():

+ /*
+ * In addition to the quals inherited from the parent, we might
+ * have securityQuals associated with this particular child node.
+ * (Currently this can only happen in appendrels originating from
+ * UNION ALL; inheritance child tables don't have their own
+ * securityQuals.) Pull any such securityQuals up into the
...
+ foreach(lc, childRTE->securityQuals)
+ {
...
+ }
+ Assert(security_level <= root->qual_security_level);
+ }

This foreach loop loops only once in the current regression tests. I checked
"Assert(childRTE->securityQuals->length == 1)" passes "make check".
I think there are no need to change codes, I state this fact only for sharing.

--
Yoshikazu Imai