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.
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.
Thanks,
Amit
