Re: Problem with default partition pruning

Sorry for being late. I found it is committed before I caught up
this thread again..

At Thu, 8 Aug 2019 14:50:54 +0900, Amit Langote <amitlangote09@gmail.com> wrote in
<CA+HiwqEmJizJ4DmuPWCL-WrHGO-hFVd08TS7HnCkSF4CyZr8tg@mail.gmail.com>
> Hi Alvaro,
> 
> On Thu, Aug 8, 2019 at 5:27 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> > On 2019-Aug-07, Simon Riggs wrote:
> > > I saw your recent commit and it scares me in various places, noted below.
> > >
> > > "Commit: Apply constraint exclusion more generally in partitioning"
> > >
> > > "This applies particularly to the default partition..."
> > >
> > > My understanding of the thread was the complaint was about removing the
> > > default partition. I would prefer to see code executed just for that case,
> > > so that people who do not define a default partition are unaffected.
> >
> > Well, as the commit message noted, it applies to other cases also, not
> > just the default partition.  The default partition just happens to be
> > the most visible case.
> 
> Just to be clear, I don't think there was any patch posted on this
> thread that was to address *non-default* partitions failing to be
> pruned by "partition pruning".  If that had been the problem, we'd be
> fixing the bugs of the partition pruning code, not apply constraint
> exclusion more generally to paper over such bugs.  I may be misreading
> what you wrote here though.
> 
> The way I interpret the "generally" in the "apply constraint exclusion
> more generally" is thus: we can't prune the default partition without
> the constraint exclusion clutches for evidently a broader sets of
> clauses than the previous design assumed.  The previous design assumed
> that only OR clauses whose arguments contradicted the parent's
> partition constraint are problematic, but evidently any clause set
> that contradicts the partition constraint is problematic.  Again, the
> problem is that it's impossible to prune the "default" partition with
> such clauses, not the *non-default* ones -- values extracted from
> contradictory clauses would not match any of the bounds so all
> non-default partitions would be pruned that way.
> 
> By the way, looking closer at the patch committed today, I realized I
> had misunderstood what you proposed as the *4th* possible place to
> move the constraint exclusion check to.  I had misread the proposal
> and thought you meant to move it outside the outermost loop of
> gen_partprune_steps_internal(), but that's not where the check is now.
> I think it's better to call predicate_refuted_by() only once by
> passing the whole list of clauses instead of for each clause
> separately.  The result would be the same but the former would be more
> efficient, because it avoids repeatedly paying the cost of setting up
> predtest.c data structures when predicate_refuted_by() is called.
> Sorry that I'm only saying this now.

+1 as I mentioned in [1].

> Also it wouldn't be incorrect to do the check only if the parent has a
> default partition.  That will also address the Simon's concern this
> might slow down the cases where this effort is useless.
> 
> I've attached a patch that does that.  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?

Also, I'd like to see comments that the partition_quals is
already varnode-fixed.

And 0002, yeah, just +1 from me.


[1] https://www.postgresql.org/message-id/20190409.173725.31175835.horiguchi.kyotaro@lab.ntt.co.jp

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center