On 2019/05/18 23:58, Tom Lane wrote:
> David Rowley <david.rowley@2ndquadrant.com> writes:
>> Thanks for working on this.
+1. Thanks a lot Tom for the cleanup.
>> I did have a look and was a bit
>> disappointed to see the code will end up calling pull_exec_paramids()
>> 3 times for any expression with exec param IDs. I can knock that down
>> to just 2 by caching the exec Param IDs of the used quals in
>> GeneratePruningStepsContext. I imagine you didn't do this since its
>> possible we reject the qual even after has_exec_param is set,
>
> Exactly.
Yeah, the point at which we can be sure that any given expression (Params,
etc.) will be in a pruning step is when we actually build the step
containing that expression. So, going over the resulting pruning steps to
collect execparamids is as good as collecting them in the context struct
(a Bitmapset field of it) as the steps are built. For example,
gen_prune_step_op() itself may add pull_exec_paramids(exprs) to
context->execparamids when building the individual steps, instead of
get_partkey_exec_paramids() going over steps and collecting them from the
individual expressions of PartitionPruneStepOp steps. However, doing that
only changes the place where that work is done, not reduce the amount of
work that needs to be done.
>> but if
>> we just delay recording the param IDs until after we're certain we're
>> using the qual then it should be safe to do this.
>
> I thought about that too, but it seems awfully messy/fragile; it's not
> clear to me at what point this code is really guaranteed to have accepted
> a clause. The recursion for AND/OR, in particular, seems to make that
> kind of questionable.
>
> If we could be sure about that I'd be inclined to apply the principle
> to the has_mutable_arg and has_exec_param flags as well as exec_paramids,
> and change the order of tests in match_clause_to_partition_key back to
> what it was, relying on the outer control layer to not propagate that
> data into the overall state until we knew whether the clause was really
> going to contribute to the steps.
Per what I mentioned above, this idea means we'll want to update all of
has_mutable_arg, has_exec_param and exec_paramids in gen_prune_step_op(),
by which point it's clear that the clause(s) having those properties
really do contribute to steps.
However, make_partitionedrel_pruneinfo() throws these steps away if it can
determine from those properties that run-time pruning will be useless.
So, I was thinking maybe there is no point in building the steps in the
first place. IOW, setting those properties in
match_clause_to_partition_key() as is done now and skipping the part of
gen_partprune_steps_internal() where clauses are turned into steps, if the
combination of context->target and the properties suggests that those step
will be useless anyway. However, inventing a new meaning for
gen_partprune_steps_internal() returning NIL isn't without problems,
because it's interpreted in a certain way when it's invoked recursively
for an individual arm of an OR clause. I couldn't find an easy way to get
around that, so I had to scrap the idea.
Thanks,
Amit
