David Rowley <david.rowley@2ndquadrant.com> writes:
> 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".
Um ... what about a merely-stable comparison operator with a Const?
> 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.
The loop over steps, per se, isn't that expensive --- but extra syscache
lookups are. Or at least that's my gut feeling about it. If we just had
match_clause_to_partition_key mark the steps as being plan-time executable
or not, we could avoid the repeat lookup.
>> * 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.
That's an interesting example, but how does it work now? I did not
observe any code in there that seems to be aware of cross-dependencies
between steps. Presumably somewhere we are combining those hash quals,
so couldn't we mark the combined step properly?
>> 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.
Yeah, I'm leery of adding much new complexity right now, but maybe
adding a flag field to save a syscache lookup wouldn't be out of line.
regards, tom lane