Fix a thinko in join_is_legal: when we decide we can implement a semijoin

tglsfdc · tglsfdc · commit a43b190e3c71 · 2009-07-19T20:32:48.000Z
by unique-ifying the RHS and then inner-joining to some other relation,
that is not grounds for violating the RHS of some other outer join.
Noticed while regression-testing new GEQO code, which will blindly follow
any path that join_is_legal says is legal, and then complain later if that
leads to a dead end.

I'm not certain that this can result in any visible failure in 8.4: the
mistake may always be masked by the fact that subsequent attempts to join
the rest of the RHS of the other join will fail.  But I'm not certain it
can't, either, and it's definitely not operating as intended.  So back-patch.

The added regression test depends on the new no-failures-allowed logic
that I'm about to commit in GEQO, so no point back-patching that.
diff --git a/src/backend/optimizer/path/joinrels.c b/src/backend/optimizer/path/joinrels.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/optimizer/path/joinrels.c,v 1.100 2009/06/11 14:48:59 momjian Exp $
+ *	  $PostgreSQL: pgsql/src/backend/optimizer/path/joinrels.c,v 1.101 2009/07/19 20:32:48 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -349,6 +349,7 @@ join_is_legal(PlannerInfo *root, RelOptInfo *rel1, RelOptInfo *rel2,
 {
 	SpecialJoinInfo *match_sjinfo;
 	bool		reversed;
+	bool		unique_ified;
 	bool		is_valid_inner;
 	ListCell   *l;
 
@@ -366,6 +367,7 @@ join_is_legal(PlannerInfo *root, RelOptInfo *rel1, RelOptInfo *rel2,
 	 */
 	match_sjinfo = NULL;
 	reversed = false;
+	unique_ified = false;
 	is_valid_inner = true;
 
 	foreach(l, root->join_info_list)
@@ -450,6 +452,7 @@ join_is_legal(PlannerInfo *root, RelOptInfo *rel1, RelOptInfo *rel2,
 				return false;	/* invalid join path */
 			match_sjinfo = sjinfo;
 			reversed = false;
+			unique_ified = true;
 		}
 		else if (sjinfo->jointype == JOIN_SEMI &&
 				 bms_equal(sjinfo->syn_righthand, rel1->relids) &&
@@ -461,6 +464,7 @@ join_is_legal(PlannerInfo *root, RelOptInfo *rel1, RelOptInfo *rel2,
 				return false;	/* invalid join path */
 			match_sjinfo = sjinfo;
 			reversed = true;
+			unique_ified = true;
 		}
 		else
 		{
@@ -510,8 +514,13 @@ join_is_legal(PlannerInfo *root, RelOptInfo *rel1, RelOptInfo *rel2,
 		}
 	}
 
-	/* Fail if violated some SJ's RHS and didn't match to another SJ */
-	if (match_sjinfo == NULL && !is_valid_inner)
+	/*
+	 * Fail if violated some SJ's RHS and didn't match to another SJ.
+	 * However, "matching" to a semijoin we are implementing by
+	 * unique-ification doesn't count (think: it's really an inner join).
+	 */
+	if (!is_valid_inner &&
+		(match_sjinfo == NULL || unique_ified))
 		return false;			/* invalid join path */
 
 	/* Otherwise, it's a valid join */
diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out
@@ -2150,6 +2150,20 @@ select count(*) from tenk1 x where
      1
 (1 row)
 
+-- try that with GEQO too
+begin;
+set geqo = on;
+set geqo_threshold = 2;
+select count(*) from tenk1 x where
+  x.unique1 in (select a.f1 from int4_tbl a,float8_tbl b where a.f1=b.f1) and
+  x.unique1 = 0 and
+  x.unique1 in (select aa.f1 from int4_tbl aa,float8_tbl bb where aa.f1=bb.f1);
+ count 
+-------
+     1
+(1 row)
+
+rollback;
 --
 -- Clean up
 --
diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql
@@ -343,6 +343,16 @@ select count(*) from tenk1 x where
   x.unique1 = 0 and
   x.unique1 in (select aa.f1 from int4_tbl aa,float8_tbl bb where aa.f1=bb.f1);
 
+-- try that with GEQO too
+begin;
+set geqo = on;
+set geqo_threshold = 2;
+select count(*) from tenk1 x where
+  x.unique1 in (select a.f1 from int4_tbl a,float8_tbl b where a.f1=b.f1) and
+  x.unique1 = 0 and
+  x.unique1 in (select aa.f1 from int4_tbl aa,float8_tbl bb where aa.f1=bb.f1);
+rollback;
+
 
 --
 -- Clean up

Original file line number	Diff line number	Diff line change
`@@ -8,7 +8,7 @@`
`8`	`8`	`*`
`9`	`9`	`*`
`10`	`10`	`* IDENTIFICATION`
`11`		`- * $PostgreSQL: pgsql/src/backend/optimizer/path/joinrels.c,v 1.100 2009/06/11 14:48:59 momjian Exp $`
	`11`	`+ * $PostgreSQL: pgsql/src/backend/optimizer/path/joinrels.c,v 1.101 2009/07/19 20:32:48 tgl Exp $`
`12`	`12`	`*`
`13`	`13`	`*-------------------------------------------------------------------------`
`14`	`14`	`*/`
`@@ -349,6 +349,7 @@ join_is_legal(PlannerInfo root, RelOptInfo rel1, RelOptInfo *rel2,`
`349`	`349`	`{`
`350`	`350`	`SpecialJoinInfo *match_sjinfo;`
`351`	`351`	`bool reversed;`
	`352`	`+ bool unique_ified;`
`352`	`353`	`bool is_valid_inner;`
`353`	`354`	`ListCell *l;`
`354`	`355`
`@@ -366,6 +367,7 @@ join_is_legal(PlannerInfo root, RelOptInfo rel1, RelOptInfo *rel2,`
`366`	`367`	`*/`
`367`	`368`	`match_sjinfo = NULL;`
`368`	`369`	`reversed = false;`
	`370`	`+ unique_ified = false;`
`369`	`371`	`is_valid_inner = true;`
`370`	`372`
`371`	`373`	`foreach(l, root->join_info_list)`
`@@ -450,6 +452,7 @@ join_is_legal(PlannerInfo root, RelOptInfo rel1, RelOptInfo *rel2,`
`450`	`452`	`return false; /* invalid join path */`
`451`	`453`	`match_sjinfo = sjinfo;`
`452`	`454`	`reversed = false;`
	`455`	`+ unique_ified = true;`
`453`	`456`	`}`
`454`	`457`	`else if (sjinfo->jointype == JOIN_SEMI &&`
`455`	`458`	`bms_equal(sjinfo->syn_righthand, rel1->relids) &&`
`@@ -461,6 +464,7 @@ join_is_legal(PlannerInfo root, RelOptInfo rel1, RelOptInfo *rel2,`
`461`	`464`	`return false; /* invalid join path */`
`462`	`465`	`match_sjinfo = sjinfo;`
`463`	`466`	`reversed = true;`
	`467`	`+ unique_ified = true;`
`464`	`468`	`}`
`465`	`469`	`else`
`466`	`470`	`{`
`@@ -510,8 +514,13 @@ join_is_legal(PlannerInfo root, RelOptInfo rel1, RelOptInfo *rel2,`
`510`	`514`	`}`
`511`	`515`	`}`
`512`	`516`
`513`		`- /* Fail if violated some SJ's RHS and didn't match to another SJ */`
`514`		`- if (match_sjinfo == NULL && !is_valid_inner)`
	`517`	`+ /*`
	`518`	`+ * Fail if violated some SJ's RHS and didn't match to another SJ.`
	`519`	`+ * However, "matching" to a semijoin we are implementing by`
	`520`	`+ * unique-ification doesn't count (think: it's really an inner join).`
	`521`	`+ */`
	`522`	`+ if (!is_valid_inner &&`
	`523`	`+ (match_sjinfo == NULL \|\| unique_ified))`
`515`	`524`	`return false; /* invalid join path */`
`516`	`525`
`517`	`526`	`/* Otherwise, it's a valid join */`