Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
> On 2019/05/14 15:09, David Rowley wrote:
>> 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?
Not sure I'll be the one to commit this, but ...
I don't much like the approach in
v1-0003-Fix-planner-to-not-prune-partitions-with-non-immu.patch
because (a) it appears to me to add duplicative expression mutability
testing, which is kind of expensive since that adds syscache lookups,
plus it adds such tests even for clauses that aren't going to match any
partition keys; and (b) it's extremely nonobvious that this restricts
plan-time pruning and not run-time pruning. I do see that
prune_append_rel_partitions is used only for the former; but you sure
as heck can't deduce that from any of its comments, so somebody might
try to use it for run-time pruning.
So I think David's got the right idea that match_clause_to_partition_key
is where to handle this, and I like the fact that his patch explicitly
represents whether we're trying to do run-time or plan-time pruning.
I agree it's kind of invasive, and I wonder why. Shouldn't the
additional flag just be a field in the "context" struct, instead of
being a separate parameter that has to be laboriously passed through
many call levels?
(For myself, I'd have made it just a bool not an enum, given that
I don't foresee a need for additional values. But that's definitely
a matter of preference.)
Also, I'm a bit confused by the fact that we seem to have a bunch
of spare-parts patches in this thread. What combination is actually
being proposed for commit?
regards, tom lane