On 2019/05/14 15:09, David Rowley wrote:
> On Tue, 14 May 2019 at 16:07, Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>>
>> The problem is different. '2018-01-01'::timestamptz' in the condition
>> datadatetime < '2018-01-01'::timestamptz as presented to
>> match_clause_to_partition_key() is indeed a Const node, making it think
>> that it's OK to prune using it, that is, with or without your patch.
>
> Looks like I misunderstood the problem.
>
> Is it not still better to ignore the unsupported quals in
> match_clause_to_partition_key() rather than crafting a new List of
> just the valid ones like you've done in your patch?
That's another way...
> I feel that what you've got spreads
> the logic out a bit too much. match_clause_to_partition_key() has
> traditionally been in charge of deciding what quals can be used and
> which ones can't. You've gone and added logic in> prune_append_rel_partitions() that makes part of this decision now
and
> that feels a bit wrong to me.
Actually, I had considered a solution like yours of distinguishing
prune_append_rel_partitions()'s invocations of gen_partprune_steps() from
make_partition_pruneinfo()'s, but thought it would be too much churn.
Also, another thing that pushed me toward the approach I took is what I
saw in predtest.c. I mentioned upthread that constraint exclusion doesn't
have this problem, that is, it knows to skip stable-valued clauses, so I
went into predicate_refuted_by() and underlings to see how they do that,
but the logic wasn't down there. It was in the following code in
relation_excluded_by_constraints():
/*
* ... We dare not make
* deductions with non-immutable functions
* ...
*/
safe_restrictions = NIL;
foreach(lc, rel->baserestrictinfo)
{
RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
if (!contain_mutable_functions((Node *) rinfo->clause))
safe_restrictions = lappend(safe_restrictions, rinfo->clause);
}
I think prune_append_rel_partitions() is playing a similar role as
relation_excluded_by_constraints(), that is, of dispatching the
appropriate clauses to the main pruning logic. So, I thought enforcing
this policy in prune_append_rel_partitions() wouldn't be such a bad idea.
> I've attached patch of how I think it should work. This includes the
> changes you made to analyze_partkey_exprs() and your tests from
> v1-0003-Fix-planner-to-not-prune-partitions-with-non-immu.patch
Thanks for updating the patch. It works now.
> Do you think it's a bad idea to do it this way?
As I mentioned above, I think this may be quite a bit of code churn for
enforcing a policy that's elsewhere enforced in a much simpler manner.
How about deferring this to committer?
Thanks,
Amit
