Problem with default partition pruning

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
89 messages Options
12345
Reply | Threaded
Open this post in threaded view
|

Re: Problem with default partition pruning

Amit Langote
On Fri, Aug 9, 2019 at 1:17 PM Amit Langote <[hidden email]> wrote:

> On Fri, Aug 9, 2019 at 12:09 PM Kyotaro Horiguchi
> <[hidden email]> wrote:
> > At Thu, 8 Aug 2019 14:50:54 +0900, Amit Langote wrote:
> > > When working on it, I realized
> > > that the way RelOptInfo.partition_qual is processed is a bit
> > > duplicative, so I created a separate patch to make that a bit more
> > > consistent.
> >
> > 0001 seems reasonable. By the way, the patch doesn't touch
> > get_relation_constraints(), but I suppose it can use the modified
> > partition constraint qual already stored in rel->partition_qual
> > in set_relation_partition_info. And we could move constifying to
> > set_rlation_partition_info?
>
> Ah, good advice.  This make partition constraint usage within the
> planner quite a bit more consistent.
Hmm, oops.  I think that judgement was a bit too rushed on my part.  I
unintentionally ended up making the partition constraint to *always*
be fetched, whereas we don't need it in most cases.  I've reverted
that change.  RelOptInfo.partition_qual is poorly named in retrospect.
:(  It's not set for all partitions, only those that are partitioned
themselves.

Attached updated patches.

Thanks,
Amit

v3-0001-Improve-RelOptInfo.partition_qual-usage.patch (5K) Download Attachment
v3-0002-Improve-constraint-exclusion-usage-in-partprune.c.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Problem with default partition pruning

Kyotaro Horiguchi-4
At Fri, 9 Aug 2019 14:02:36 +0900, Amit Langote <[hidden email]> wrote in <[hidden email]>

> On Fri, Aug 9, 2019 at 1:17 PM Amit Langote <[hidden email]> wrote:
> > On Fri, Aug 9, 2019 at 12:09 PM Kyotaro Horiguchi
> > <[hidden email]> wrote:
> > > At Thu, 8 Aug 2019 14:50:54 +0900, Amit Langote wrote:
> > > > When working on it, I realized
> > > > that the way RelOptInfo.partition_qual is processed is a bit
> > > > duplicative, so I created a separate patch to make that a bit more
> > > > consistent.
> > >
> > > 0001 seems reasonable. By the way, the patch doesn't touch
> > > get_relation_constraints(), but I suppose it can use the modified
> > > partition constraint qual already stored in rel->partition_qual
> > > in set_relation_partition_info. And we could move constifying to
> > > set_rlation_partition_info?
> >
> > Ah, good advice.  This make partition constraint usage within the
> > planner quite a bit more consistent.
>
> Hmm, oops.  I think that judgement was a bit too rushed on my part.  I
> unintentionally ended up making the partition constraint to *always*
> be fetched, whereas we don't need it in most cases.  I've reverted

(v2 has been withdrawn before I see it:p)

Yeah, I agreed. It is needed only by (sub)partition parents.

> that change.  RelOptInfo.partition_qual is poorly named in retrospect.
> :(  It's not set for all partitions, only those that are partitioned
> themselves.
>
> Attached updated patches.

+++ b/src/backend/optimizer/util/plancat.c
@@ -1267,10 +1267,14 @@ get_relation_constraints(PlannerInfo *root,
    */
   if (include_partition && relation->rd_rel->relispartition)
   {
...
+    else
     {
+      /* Nope, fetch from the relcache. */

I seems to me that include_partition is true both and only for
modern and old-fasheoned partition parents.
set_relation_partition_info() is currently called only for modern
partition parents. If we need that at the place above,
set_relation_partition_info can be called also for old-fashioned
partition parent, and get_relation_constraints may forget the
else case in a broad way.


+      /* Nope, fetch from the relcache. */
+      List       *pcqual = RelationGetPartitionQual(relation);

If the comment above is right, This would be duplicate. What we
should do instaed is only eval_const_expression. And we could
move it to set_relation_partition_info completely. Consitify must
be useful in both cases.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center


Reply | Threaded
Open this post in threaded view
|

Re: Problem with default partition pruning

Amit Langote
Thanks for the comments.

On Fri, Aug 9, 2019 at 2:44 PM Kyotaro Horiguchi
<[hidden email]> wrote:

> +++ b/src/backend/optimizer/util/plancat.c
> @@ -1267,10 +1267,14 @@ get_relation_constraints(PlannerInfo *root,
>     */
>    if (include_partition && relation->rd_rel->relispartition)
>    {
> ...
> +    else
>      {
> +      /* Nope, fetch from the relcache. */
>
> I seems to me that include_partition is true both and only for
> modern and old-fasheoned partition parents.
> set_relation_partition_info() is currently called only for modern
> partition parents. If we need that at the place above,
> set_relation_partition_info can be called also for old-fashioned
> partition parent, and get_relation_constraints may forget the
> else case in a broad way.

"include_partition" doesn't have anything to do with what kind the
partition parent is.  It is true when the input relation that is a
partition is directly mentioned in the query (RELOPT_BASEREL) and
constraint_exclusion is on (inheritance_planner considerations makes
the actual code a bit hard to follow but we'll hopefully simplify that
in the near future).  That is also the only case where we need to
consider the partition constraint when doing constraint exclusion.
Regarding how this relates to partition_qual:

* get_relation_constraints() can use it if it's set, which would be
the case if the partition in question is partitioned itself

* It wouldn't be set if the partition in question is a leaf partition,
so it will have to get it directly from the relcache

> +      /* Nope, fetch from the relcache. */
> +      List       *pcqual = RelationGetPartitionQual(relation);
>
> If the comment above is right, This would be duplicate. What we
> should do instaed is only eval_const_expression. And we could
> move it to set_relation_partition_info completely. Consitify must
> be useful in both cases.

As described above, this block of code is not really duplicative in
the sense that when it runs, that would be the first time in a query
to fetch the partition constraint of the relation in question.

Also, note that expression_planner() calls eval_const_expressions(),
so constification happens in both cases.  I guess different places
have grown different styles of processing constraint expressions as
the APIs have evolved over time.

Thanks,
Amit


Reply | Threaded
Open this post in threaded view
|

Re: Problem with default partition pruning

Alvaro Herrera-9
In reply to this post by Amit Langote
On 2019-Aug-09, Amit Langote wrote:

> Hmm, oops.  I think that judgement was a bit too rushed on my part.  I
> unintentionally ended up making the partition constraint to *always*
> be fetched, whereas we don't need it in most cases.  I've reverted
> that change.

Yeah, I was quite confused about this point yesterday while I was trying
to make sense of your patches.

> RelOptInfo.partition_qual is poorly named in retrospect.
> :(  It's not set for all partitions, only those that are partitioned
> themselves.

Oh.  Hmm, I think this realization further clarifies things.

Since we're only changing this in the master branch anyway, maybe we can
find a better name for it.

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Reply | Threaded
Open this post in threaded view
|

Re: Problem with default partition pruning

Alvaro Herrera-9
In reply to this post by Amit Langote
v3-0001 still seems to leave things a bit duplicative.  I think we can
make it better if we move the logic to set RelOptInfo->partition_qual to
a separate routine (set_baserel_partition_constraint mirroring the
existing set_baserel_partition_key_exprs), and then call that from both
places that need access to partition_qual.

So I propose that the attached v4 patch should be the final form of this
(also rebased across today's list_concat API change).  I verified that
constraint exclusion is not being called by partprune unless a default
partition exists (thanks errbacktrace()); I think that should appease
Simon's performance concern for the most common case of default
partition not existing.

I think I was not really understanding the comments being added by
Amit's v3, so I reworded them.  I hope I understood the intent of the
code correctly.

I'm not comfortable with RelOptInfo->partition_qual.  But I'd rather
leave that for another time.

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

v4-0001-Rejigger-code.patch (7K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Problem with default partition pruning

Simon Riggs
On Mon, 12 Aug 2019 at 18:45, Alvaro Herrera <[hidden email]> wrote:
I think that should appease
Simon's performance concern for the most common case of default
partition not existing.

Much appreciated, thank you. 

--
Simon Riggs                http://www.2ndQuadrant.com/
PostgreSQL Solutions for the Enterprise
Reply | Threaded
Open this post in threaded view
|

Re: Problem with default partition pruning

Amit Langote
In reply to this post by Alvaro Herrera-9
Hi Alvaro,

On Tue, Aug 13, 2019 at 2:45 AM Alvaro Herrera <[hidden email]> wrote:

> v3-0001 still seems to leave things a bit duplicative.  I think we can
> make it better if we move the logic to set RelOptInfo->partition_qual to
> a separate routine (set_baserel_partition_constraint mirroring the
> existing set_baserel_partition_key_exprs), and then call that from both
> places that need access to partition_qual.
>
> So I propose that the attached v4 patch should be the final form of this
> (also rebased across today's list_concat API change).  I verified that
> constraint exclusion is not being called by partprune unless a default
> partition exists (thanks errbacktrace()); I think that should appease
> Simon's performance concern for the most common case of default
> partition not existing.
>
> I think I was not really understanding the comments being added by
> Amit's v3, so I reworded them.  I hope I understood the intent of the
> code correctly.

Thanks a lot for revising.  Looks neat, except:

+     * This is a measure of last resort only to be used because the default
+     * partition cannot be pruned using the steps; regular pruning, which is
+     * cheaper, is sufficient when no default partition exists.

This text appears to imply that the default can *never* be pruned with
steps.  Maybe, the first sentence should read something like: "...the
default cannot be pruned using the steps generated from clauses that
contradict the parent's partition constraint".

> I'm not comfortable with RelOptInfo->partition_qual.  But I'd rather
> leave that for another time.

Sure.

Regards,
Amit


Reply | Threaded
Open this post in threaded view
|

Re: Problem with default partition pruning

Alvaro Herrera-9
On 2019-Aug-13, Amit Langote wrote:

> Thanks a lot for revising.  Looks neat, except:
>
> +     * This is a measure of last resort only to be used because the default
> +     * partition cannot be pruned using the steps; regular pruning, which is
> +     * cheaper, is sufficient when no default partition exists.
>
> This text appears to imply that the default can *never* be pruned with
> steps.  Maybe, the first sentence should read something like: "...the
> default cannot be pruned using the steps generated from clauses that
> contradict the parent's partition constraint".

Thanks!  I have pushed it with this change.

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Reply | Threaded
Open this post in threaded view
|

Re: Problem with default partition pruning

Amit Langote
On Wed, Aug 14, 2019 at 12:25 AM Alvaro Herrera
<[hidden email]> wrote:

> On 2019-Aug-13, Amit Langote wrote:
>
> > Thanks a lot for revising.  Looks neat, except:
> >
> > +     * This is a measure of last resort only to be used because the default
> > +     * partition cannot be pruned using the steps; regular pruning, which is
> > +     * cheaper, is sufficient when no default partition exists.
> >
> > This text appears to imply that the default can *never* be pruned with
> > steps.  Maybe, the first sentence should read something like: "...the
> > default cannot be pruned using the steps generated from clauses that
> > contradict the parent's partition constraint".
>
> Thanks!  I have pushed it with this change.

Thank you Alvaro.  This takes care of all the issues around default
partition pruning reported on this thread.  Thanks everyone.

Regards,
Amit


12345