Re: inconsistent results querying table partitioned by date - Mailing list pgsql-bugs

On Thu, 16 May 2019 at 05:38, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> David Rowley <david.rowley@2ndquadrant.com> writes:
> > [ dont_prune_with_nonimmutable_opfuncs_during_planning_v3.patch ]
>
> I started working through this, and changed some comments I thought could
> be improved, and noticed that you'd missed out making the same changes
> for ScalarArrayOp so I did that.  However, when I got to the changes in
> analyze_partkey_exprs, my bogometer went off.  Why do we need to recheck
> this, in fact why does that function exist at all?  ISTM if we have used
> a pruning qual at plan time there is no need to check it again at runtime;
> therefore, it shouldn't even be in the list that analyze_partkey_exprs
> is looking at.

Thinking about this more, if we're now making it so a forplanner =
true set of steps requires all values being compared to the partition
key to be Const, then anything that's not Const must require run-time
pruning. That means the:

else if (func_volatile(fnoid) != PROVOLATILE_IMMUTABLE)

in analyze_partkey_exprs() can simply become "else".

> I am wondering therefore whether we shouldn't do one of these two
> things:
>
> * Teach match_clause_to_partition_key to not emit plan-time-usable
> quals at all if it is called with forplanner = false, or

That's not really possible.  Imagine a table partitioned by HASH(a,b)
and WHERE a = $1 and b = 10;  we can't do any pruning during planning
here, and if we only generate steps containing "a = $1" for run-time,
then we can't do anything there either.

> * Add a bool field to PartitionPruneStep indicating whether the step
> can be used at plan time, and use that to skip redundant checks at run
> time.  This would make more sense if we can then fix things so that
> we only run match_clause_to_partition_key once per qual clause ---
> I gather that it's now being run twice, which seems pretty inefficient.

I think it would be nicer if we made match_clause_to_partition_key()
do everything that analyze_partkey_exprs() currently does, but if we
change that "else if" to just "else" then the only advantage is not
having to make another pass over the pruning steps.  We'd have to make
it so match_clause_to_partition_key() did all the pull_exec_paramids()
stuff and add some new fields to PartitionPruneStepOp to store that
info.  We'd also need to store hasexecparam in the
PartitionPruneStepOp and change the signature of
partkey_datum_from_expr() so we could pass that down as we'd no longer
have context->exprhasexecparam... Well, we could keep that, but if
we're trying to not loop over the steps again then we'll need to store
that value in the step when it's created rather than put it in
PartitionPruneContext.

> Perhaps this implies too much code churn/restructuring to be reasonable
> to consider right now, but I thought I'd ask.  It sure seems like this
> work is being done in a pretty inefficient and duplicative way.

I certainly think the above would be cleaner and I don't mind working
on it, but if it's too much churn for this time of year then I'd
rather not waste time writing a patch that's going to get rejected.

-- 
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services