Thanks for the updated patches.
On 2019/05/15 8:52, David Rowley wrote:
> On Wed, 15 May 2019 at 08:58, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> 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?
>
> Thanks for having a look. I originally stayed clear of "context"
> since it looks more like how we output the pruning steps, rather than
> a context as to how they should be generated. But it's true, using it
> saves having to pass the extra argument around, so I made that change.
>
>> (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.)
>
> I've changed to bool, mostly because it makes the patch a bit smaller.
Looks good. Thanks for tweaking many comments for clarity, except this
one could be improved a bit more:
+ * 'forplanner' must be true when being called from the query planner.
This description of 'forplanner' might make it sound like it's redundant
(always true), because gen_partprune_steps() is only called from the
planner today. How about making it say the same thing as the comment next
to its definition in GeneratePruningStepsContext struct):
'forplanner' must be true if generating steps to be used during query
planning.
>> 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?
>
> I was also confused at the extra two patches Amit posted. The extra
> one for the tests didn't look very valid to me, as I mentioned
> yesterday.
As I said in my previous reply, 0001 contains wrong expected output
because of the other bugs discussed earlier. I guess that's not very helpful.
Anyway, I'm attaching a squashed version of those two patches as
0001-Fix-bugs-in-pruning-with-composite-range-partition-k.patch. It
applies as-is to both master and v11 branches.
> I propose the attached for master, and I'll post something for v11 shortly.
Thanks. I'm re-attaching both of your patches here just to have all the
patches in one place.
Thanks,
Amit