Re: inconsistent results querying table partitioned by date - Mailing list pgsql-bugs

On Mon, 13 May 2019 at 17:40, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
>
> On 2019/05/11 6:05, Tom Lane wrote:
> > regression=# explain select * from dataid where id=1 and datadatetime < '2018-01-01'::timestamptz;
> >                                                QUERY PLAN
> > --------------------------------------------------------------------------------------------------------
> >  Bitmap Heap Scan on dataid_default  (cost=4.18..11.30 rows=3 width=12)
> >    Recheck Cond: ((id = 1) AND (datadatetime < '2018-01-01 00:00:00-05'::timestamp with time zone))
> >    ->  Bitmap Index Scan on dataid_default_pkey  (cost=0.00..4.18 rows=3 width=0)
> >          Index Cond: ((id = 1) AND (datadatetime < '2018-01-01 00:00:00-05'::timestamp with time zone))
> > (4 rows)
> >
> > That's not fine.  What we have here is a "timestamp < timestamptz"
> > operator, which is only stable, therefore it might give different
> > results at runtime than at plan time.  You can't make plan-time
> > pruning decisions with that.  What we should have gotten here was
> > an Append node that could do run-time pruning.
>
> You're right.  It seems that prune_append_rel_partitions() is forgetting
> to filter mutable clauses from rel->baserestrictinfo, like
> relation_excluded_by_constraints() does.  I fixed that in the attached
> 0003 patch, which also adds a test for this scenario.  I needed to also
> tweak run-time pruning support code a bit so that it considers the cases
> involving mutable functions as requiring (startup) run-time pruning, in
> addition to the cases with mutable expressions.  Adding David if he wants
> to comment.

Yeah. I don't think you're going about this the right way.  I don't
really see why we need to make any changes to the run-time pruning
code here, that part seems fine to me.  The problem seems to be that
match_clause_to_partition_key() thinks it can use a non-const
expression to compare to the partition key.  All immutable function
calls will already be folded to constants by this time, so what's
wrong with just insisting that the value being compared to the
partition key is a constant when generating steps during planning?
When we generate steps for run-time pruning we can use stable function
return values, but obviously still not volatile functions.   So isn't
the fix just to provide gen_partprune_steps with some sort of context
as to the environment that it's generating steps for?  Fixing it by
insisting the planner pruning only uses consts saves us some cycles
where we check for Vars and volatile expressions too. That's a pretty
good saving when you consider the fact that the expression sometimes
could be fairly complex.

I did it that way in the attached then tried running with your tests
and got failures.  After looking at those your tests look like they're
expecting the wrong results.

E.g

+explain (analyze, costs off, summary off, timing off) select * from
mc3p where a = 1 and abs(b) < (select 2);
+                       QUERY PLAN
+--------------------------------------------------------
+ Append (actual rows=0 loops=1)
+   InitPlan 1 (returns $0)
+     ->  Result (actual rows=1 loops=1)
+   ->  Seq Scan on mc3p0 (actual rows=0 loops=1)
+         Filter: ((a = 1) AND (abs(b) < $0))
+   ->  Seq Scan on mc3p_default (actual rows=0 loops=1)
+         Filter: ((a = 1) AND (abs(b) < $0))
+(7 rows)

to which, it does not seem very good that you pruned "mc3p1", since
(with my patch)

postgres=# insert into mc3p (a,b,c) values(1,1,1);
INSERT 0 1
postgres=# select tableoid::regclass,* from mc3p where a = 1 and
abs(b) < (select 2);
 tableoid | a | b | c
----------+---+---+---
 mc3p1    | 1 | 1 | 1
(1 row)

If those tests were passing in your code then you'd have missed that
row... Oops.

I've attached how I think the fix should look and included some tests
too.  I was a bit unsure if it was worth adding the new enum or not.
Probably could be just a bool, but I thought enum as it makes looking
at function calls easier on the human eye.  I also included a test
that ensures an immutable function's value is used for pruning during
planning...  Half of me thinks that test is pretty useless since the
return value is converted to a const well before pruning, but I left
it there anyway...

I've not done the PG11 version of the patch but I can do it if the
PG12 one looks okay.

-- 
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services