From: | Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> |
---|---|
To: | David Rowley <david(dot)rowley(at)2ndquadrant(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>, ajax(at)tvsquared(dot)com, PostgreSQL mailing lists <pgsql-bugs(at)lists(dot)postgresql(dot)org> |
Subject: | Re: inconsistent results querying table partitioned by date |
Date: | 2019-05-15 02:10:24 |
Message-ID: | 588f56cb-aaed-14b0-8a21-13486bd1c706@lab.ntt.co.jp |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-bugs |
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(at)sss(dot)pgh(dot)pa(dot)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
Attachment | Content-Type | Size |
---|---|---|
0001-Fix-bugs-in-pruning-with-composite-range-partition-k.patch | text/plain | 9.5 KB |
dont_prune_with_nonimmutable_opfuncs_during_planning_v2.patch | text/plain | 10.0 KB |
pg11_dont_prune_with_nonimmutable_opfuncs_during_planning_v2.patch | text/plain | 9.8 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | David Rowley | 2019-05-15 02:20:14 | Re: inconsistent results querying table partitioned by date |
Previous Message | Amit Langote | 2019-05-15 01:22:55 | Re: inconsistent results querying table partitioned by date |