From: "Yuzuko Hosoya" <hosoya.yuzuko@lab.ntt.co.jp>
References: <9dc22159-4869-dd2b-feb5-c39af3c01a7e@lab.ntt.co.jp>
	<00c101d4ede0$babd4390$3037cab0$@lab.ntt.co.jp>
	<20190409.102848.252476604.horiguchi.kyotaro@lab.ntt.co.jp>
 <20190409.103317.20446083.horiguchi.kyotaro@lab.ntt.co.jp>
In-Reply-To: <20190409.103317.20446083.horiguchi.kyotaro@lab.ntt.co.jp>
Subject: RE: Problem with default partition pruning
Date: Tue, 9 Apr 2019 16:41:47 +0900
Message-ID: <00cf01d4eea7$afa43370$0eec9a50$@lab.ntt.co.jp>
MIME-Version: 1.0
Content-Type: multipart/mixed;
	boundary="----=_NextPart_000_00D0_01D4EEF3.1F8C2990"
Content-language: ja
Thread-index: AQF9mTaRY9vJgw/ba5uOY8SApfldLQGdaKIsAX6Rm2kCQJtMQaa3LdZw
To: "'Kyotaro HORIGUCHI'" <horiguchi.kyotaro@lab.ntt.co.jp>
Cc: <Langote_Amit_f8@lab.ntt.co.jp>, <thibaut.madelaine@dalibo.com>,
        <imai.yoshikazu@jp.fujitsu.com>, <pgsql-hackers@lists.postgresql.org>
Precedence: bulk

This is a multipart message in MIME format.

------=_NextPart_000_00D0_01D4EEF3.1F8C2990
Content-Type: text/plain;
	charset="us-ascii"
Content-Transfer-Encoding: 7bit

Horiguchi-san,

Thanks for your comments.

> -----Original Message-----
> From: Kyotaro HORIGUCHI [mailto:horiguchi.kyotaro@lab.ntt.co.jp]
> Sent: Tuesday, April 09, 2019 10:33 AM
> To: hosoya.yuzuko@lab.ntt.co.jp
> Cc: Langote_Amit_f8@lab.ntt.co.jp; thibaut.madelaine@dalibo.com; imai.yoshikazu@jp.fujitsu.com;
> pgsql-hackers@lists.postgresql.org
> Subject: Re: Problem with default partition pruning
> 
> Sigh..
> 
> At Tue, 09 Apr 2019 10:28:48 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI
> <horiguchi.kyotaro@lab.ntt.co.jp> wrote in
> <20190409.102848.252476604.horiguchi.kyotaro@lab.ntt.co.jp>
> > As the second thought. Partition constraint is not constraint
> > expression so that's fair to apply partqual ignoring
> > constraint_exclusion. The variable is set false to skip useless
> > expression evaluation on all partitions, but partqual should be
> > evaluated just once.  Sorry for my confusion.
> >
> > So still it is wrong that the new code is added in
> > gen_partprune_steps_internal.
> 
> So still it is wrong that the new code is added at the beginning of the loop on clauses in
> gen_partprune_steps_internal.
> 
> >                               If partqual results true and the clause
> > is long, the partqual is evaluated uselessly at every recursion.
> >
> > Maybe we should do that when we find that the current clause doesn't
> > match part attributes. Specifically just after the for loop "for (i =
> > 0 ; i < part_scheme->partnattrs; i++)".
>
I think we should check whether WHERE clause contradicts partition
constraint even when the clause matches part attributes.  So I moved
"if (partqual)" block to the beginning of the loop you mentioned. 

I'm attaching the latest version.  Could you please check it again?

Best regards,
Yuzuko Hosoya

------=_NextPart_000_00D0_01D4EEF3.1F8C2990
Content-Type: application/octet-stream;
	name="v4_ignore_contradictory_where_clauses_at_partprune_step.patch"
Content-Transfer-Encoding: quoted-printable
Content-Disposition: attachment;
	filename="v4_ignore_contradictory_where_clauses_at_partprune_step.patch"

diff --git a/src/backend/partitioning/partprune.c =
b/src/backend/partitioning/partprune.c=0A=
index 5cd740a..11e3e7a 100644=0A=
--- a/src/backend/partitioning/partprune.c=0A=
+++ b/src/backend/partitioning/partprune.c=0A=
@@ -850,32 +850,13 @@ =
gen_partprune_steps_internal(GeneratePruningStepsContext *context,=0A=
 						 * an arg.  To indicate that to the pruning code, we=0A=
 						 * must construct a dummy PartitionPruneStepCombine=0A=
 						 * whose source_stepids is set to an empty List.=0A=
-						 * However, if we can prove using constraint exclusion=0A=
-						 * that the clause refutes the table's partition=0A=
-						 * constraint (if it's sub-partitioned), we need not=0A=
-						 * bother with that.  That is, we effectively ignore=0A=
-						 * this OR arm.=0A=
 						 */=0A=
-						List	   *partconstr =3D rel->partition_qual;=0A=
 						PartitionPruneStep *orstep;=0A=
 =0A=
 						/* Just ignore this argument. */=0A=
 						if (arg_contradictory)=0A=
 							continue;=0A=
 =0A=
-						if (partconstr)=0A=
-						{=0A=
-							partconstr =3D (List *)=0A=
-								expression_planner((Expr *) partconstr);=0A=
-							if (rel->relid !=3D 1)=0A=
-								ChangeVarNodes((Node *) partconstr, 1,=0A=
-											   rel->relid, 0);=0A=
-							if (predicate_refuted_by(partconstr,=0A=
-													 list_make1(arg),=0A=
-													 false))=0A=
-								continue;=0A=
-						}=0A=
-=0A=
 						orstep =3D gen_prune_step_combine(context, NIL,=0A=
 														PARTPRUNE_COMBINE_UNION);=0A=
 						arg_stepids =3D lappend_int(arg_stepids, orstep->step_id);=0A=
@@ -951,6 +932,30 @@ =
gen_partprune_steps_internal(GeneratePruningStepsContext *context,=0A=
 			bool		clause_is_not_null =3D false;=0A=
 			PartClauseInfo *pc =3D NULL;=0A=
 			List	   *clause_steps =3D NIL;=0A=
+			List	   *partconstr =3D rel->partition_qual;=0A=
+=0A=
+			/*=0A=
+			 * If this clause can be proved false for this partition,=0A=
+			 * given its partition constraint, we can ignore it,=0A=
+			 * that is not try to pass it to the pruning code.=0A=
+			 * We should do that especially to avoid pruning code=0A=
+			 * wrongly failing to prune the default partition.=0A=
+			 */=0A=
+			if (partconstr)=0A=
+			{=0A=
+				partconstr =3D (List *)=0A=
+					expression_planner((Expr *) partconstr);=0A=
+				if (rel->relid !=3D 1)=0A=
+					ChangeVarNodes((Node *) partconstr, 1,=0A=
+								   rel->relid, 0);=0A=
+				if (predicate_refuted_by(partconstr,=0A=
+										 list_make1(clause),=0A=
+										 false))=0A=
+				{=0A=
+					*contradictory =3D true;=0A=
+					return NIL;=0A=
+				}=0A=
+			}=0A=
 =0A=
 			switch (match_clause_to_partition_key(rel, context,=0A=
 												  clause, partkey, i,=0A=
diff --git a/src/test/regress/expected/partition_prune.out =
b/src/test/regress/expected/partition_prune.out=0A=
index b46951c..716155d 100644=0A=
--- a/src/test/regress/expected/partition_prune.out=0A=
+++ b/src/test/regress/expected/partition_prune.out=0A=
@@ -592,6 +592,24 @@ explain (costs off) select * from rlp where a < 1 =
or (a > 20 and a < 25);=0A=
          Filter: ((a < 1) OR ((a > 20) AND (a < 25)))=0A=
 (5 rows)=0A=
 =0A=
+-- where clause contradicts sub-partition's constraint =0A=
+explain (costs off) select * from rlp where a =3D 20 or a =3D 40;=0A=
+               QUERY PLAN               =0A=
+----------------------------------------=0A=
+ Append=0A=
+   ->  Seq Scan on rlp4_1=0A=
+         Filter: ((a =3D 20) OR (a =3D 40))=0A=
+   ->  Seq Scan on rlp5_default=0A=
+         Filter: ((a =3D 20) OR (a =3D 40))=0A=
+(5 rows)=0A=
+=0A=
+explain (costs off) select * from rlp3 where a =3D 20;   /* empty */=0A=
+        QUERY PLAN        =0A=
+--------------------------=0A=
+ Result=0A=
+   One-Time Filter: false=0A=
+(2 rows)=0A=
+=0A=
 -- redundant clauses are eliminated=0A=
 explain (costs off) select * from rlp where a > 1 and a =3D 10;	/* only =
default */=0A=
             QUERY PLAN            =0A=
diff --git a/src/test/regress/sql/partition_prune.sql =
b/src/test/regress/sql/partition_prune.sql=0A=
index abc6d9e..a5e8109 100644=0A=
--- a/src/test/regress/sql/partition_prune.sql=0A=
+++ b/src/test/regress/sql/partition_prune.sql=0A=
@@ -85,6 +85,10 @@ explain (costs off) select * from rlp where a =3D 29;=0A=
 explain (costs off) select * from rlp where a >=3D 29;=0A=
 explain (costs off) select * from rlp where a < 1 or (a > 20 and a < =
25);=0A=
 =0A=
+-- where clause contradicts sub-partition's constraint=0A=
+explain (costs off) select * from rlp where a =3D 20 or a =3D 40;=0A=
+explain (costs off) select * from rlp3 where a =3D 20;   /* empty */=0A=
+=0A=
 -- redundant clauses are eliminated=0A=
 explain (costs off) select * from rlp where a > 1 and a =3D 10;	/* only =
default */=0A=
 explain (costs off) select * from rlp where a > 1 and a >=3D15;	/* rlp3 =
onwards, including default */=0A=

------=_NextPart_000_00D0_01D4EEF3.1F8C2990--