Re: inconsistent results querying table partitioned by date

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

In response to

Responses

Browse pgsql-bugs by date

  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