Re: inconsistent results querying table partitioned by date

On 2019/05/14 15:09, David Rowley wrote:
> On Tue, 14 May 2019 at 16:07, Amit Langote
> <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)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