Re: speeding up planning with partitions

Imai-san,

Thanks for the review again.

On 2018/12/05 11:29, Imai, Yoshikazu wrote:
> 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/

Fixed all of 1-5.

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

Seems like an existing comment bug. Why don't you send a patch as you
discovered it? :)

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

I realized that bms_del_member statement is unnecessary. I've revised the
comment describing live_parts to say that it contains indexes into
part_rels array of the non-NULL RelOptInfos contained in it, that is,
RelOptInfos of un-pruned partitions (rest of the entries are NULL.)
Un-pruned partitions may become dummy due to contradictory constraints or
constraint exclusion using normal CHECK constraints later and whether it's
dummy is checked properly by functions that iterate over live_parts.

> 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".

You're right. I think that may not have been true in some version of the
patch that I didn't share on the list, but it is with the latest patch.
I've removed that block of code and adjusted nearby comments to mention
that targetlist may change during query_planner.

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

Thanks for the information. There aren't any changes to the code itself
due to this patch, just moved from one place to another.

Attached updated patches. I have a few other changes in mind to make to
0001 such that the range table in each child's version of Query contains
only that child table in place of the original target relation, instead of
*all* child tables which is the current behavior. The current behavior
makes range_table_mutator a big bottleneck when the number of un-pruned
target children is large. But I'm saving it for the next week so that I
can prepare for the PGConf.ASIA that's starting on Monday next week. See
you there. :)

Thanks,
Amit