Check the validity of commutators for merge/hash clauses

When creating merge or hash join plans in createplan.c, the merge or
hash clauses may need to get commuted to ensure that the outer var is
on the left and the inner var is on the right if they are not already
in the expected form.  This requires that their operators have
commutators.  Failing to find a commutator at this stage would result
in 'ERROR: could not find commutator for operator xxx', with no
opportunity to select an alternative plan.

Typically, this is not an issue because mergejoinable or hashable
operators are expected to always have valid commutators.  But in some
artificial cases this assumption may not hold true.  Therefore, here
in this patch we check the validity of commutators for clauses in the
form "inner op outer" when selecting mergejoin/hash clauses, and
consider a clause unusable for the current pair of outer and inner
relations if it lacks a commutator.

There are not (and should not be) any such operators built into
Postgres that are mergejoinable or hashable but have no commutators;
so we leverage the alias type 'int8alias1' created in equivclass.sql
to build the test case.  This is why the test case is included in
equivclass.sql rather than in join.sql.

Although this is arguably a bug fix, it cannot be reproduced without
installing an incomplete opclass, which is unlikely to happen in
practice, so no back-patch.

Reported-by: Alexander Pyhalov
Author: Richard Guo
Reviewed-by: Tom Lane
Discussion: https://postgr.es/m/c59ec04a2fef94d9ffc35a9b17dfc081@postgrespro.ru

diff --git a/src/backend/optimizer/path/joinpath.c b/src/backend/optimizer/path/joinpath.c
index b0e8c94dfc37ddfd77f0acb0329ec3246b42ad46..a244300463c3d23e8870fabbc08b54bcf947625d 100644 (file)
--- a/src/backend/optimizer/path/joinpath.c
+++ b/src/backend/optimizer/path/joinpath.c
@@ -25,6 +25,7 @@
 #include "optimizer/paths.h"
 #include "optimizer/placeholder.h"
 #include "optimizer/planmain.h"
+#include "utils/lsyscache.h"
 #include "utils/typcache.h"
 
 /* Hook for plugins to get control in add_paths_to_joinrel() */
@@ -2266,6 +2267,20 @@ hash_inner_and_outer(PlannerInfo *root,
                if (!clause_sides_match_join(restrictinfo, outerrel, innerrel))
                        continue;                       /* no good for these input relations */
 
+               /*
+                * If clause has the form "inner op outer", check if its operator has
+                * valid commutator.  This is necessary because hashclauses in this
+                * form will get commuted in createplan.c to put the outer var on the
+                * left (see get_switched_clauses).  This probably shouldn't ever
+                * fail, since hashable operators ought to have commutators, but be
+                * paranoid.
+                *
+                * The clause being hashjoinable indicates that it's an OpExpr.
+                */
+               if (!restrictinfo->outer_is_left &&
+                       !OidIsValid(get_commutator(castNode(OpExpr, restrictinfo->clause)->opno)))
+                       continue;
+
                hashclauses = lappend(hashclauses, restrictinfo);
        }
 
@@ -2540,6 +2555,23 @@ select_mergejoin_clauses(PlannerInfo *root,
                        continue;                       /* no good for these input relations */
                }
 
+               /*
+                * If clause has the form "inner op outer", check if its operator has
+                * valid commutator.  This is necessary because mergejoin clauses in
+                * this form will get commuted in createplan.c to put the outer var on
+                * the left (see get_switched_clauses).  This probably shouldn't ever
+                * fail, since mergejoinable operators ought to have commutators, but
+                * be paranoid.
+                *
+                * The clause being mergejoinable indicates that it's an OpExpr.
+                */
+               if (!restrictinfo->outer_is_left &&
+                       !OidIsValid(get_commutator(castNode(OpExpr, restrictinfo->clause)->opno)))
+               {
+                       have_nonmergeable_joinclause = true;
+                       continue;
+               }
+
                /*
                 * Insist that each side have a non-redundant eclass.  This
                 * restriction is needed because various bits of the planner expect

diff --git a/src/test/regress/expected/equivclass.out b/src/test/regress/expected/equivclass.out
index 126f7047fed3abbf82beca04233b4f2e9ba311cb..a328164fe0f3952ecf26cfcf18590d330c217b4e 100644 (file)
--- a/src/test/regress/expected/equivclass.out
+++ b/src/test/regress/expected/equivclass.out
@@ -451,3 +451,51 @@ explain (costs off)  -- this should not require a sort
    Filter: (f1 = 'foo'::name)
 (2 rows)
 
+--
+-- test handling of merge/hash clauses that do not have valid commutators
+--
+-- There are not (and should not be) any such operators built into Postgres
+-- that are mergejoinable or hashable but have no commutators; so we leverage
+-- the alias type 'int8alias1' created in this file to conduct the tests.
+-- That's why this test is included here rather than in join.sql.
+begin;
+create table tbl_nocom(a int8, b int8alias1);
+-- check that non-commutable merge clauses do not lead to error
+set enable_hashjoin to off;
+set enable_mergejoin to on;
+explain (costs off)
+select * from tbl_nocom t1 full join tbl_nocom t2 on t2.a = t1.b;
+              QUERY PLAN              
+--------------------------------------
+ Merge Full Join
+   Merge Cond: (t2.a = t1.b)
+   ->  Sort
+         Sort Key: t2.a
+         ->  Seq Scan on tbl_nocom t2
+   ->  Sort
+         Sort Key: t1.b USING <
+         ->  Seq Scan on tbl_nocom t1
+(8 rows)
+
+-- check that non-commutable hash clauses do not lead to error
+alter operator = (int8, int8alias1) set (hashes);
+alter operator family integer_ops using hash add
+  operator 1 = (int8, int8alias1);
+create function hashint8alias1(int8alias1) returns int
+  strict immutable language internal as 'hashint8';
+alter operator family integer_ops using hash add
+  function 1 hashint8alias1(int8alias1);
+set enable_hashjoin to on;
+set enable_mergejoin to off;
+explain (costs off)
+select * from tbl_nocom t1 full join tbl_nocom t2 on t2.a = t1.b;
+              QUERY PLAN              
+--------------------------------------
+ Hash Full Join
+   Hash Cond: (t2.a = t1.b)
+   ->  Seq Scan on tbl_nocom t2
+   ->  Hash
+         ->  Seq Scan on tbl_nocom t1
+(5 rows)
+
+abort;

diff --git a/src/test/regress/sql/equivclass.sql b/src/test/regress/sql/equivclass.sql
index 247b0a31055769749d9ab3b991058cc43c2d27d8..28ed7910d014e44bca138c7b424bc3264753f3d5 100644 (file)
--- a/src/test/regress/sql/equivclass.sql
+++ b/src/test/regress/sql/equivclass.sql
@@ -269,3 +269,37 @@ create temp view overview as
   select f1::information_schema.sql_identifier as sqli, f2 from undername;
 explain (costs off)  -- this should not require a sort
   select * from overview where sqli = 'foo' order by sqli;
+
+--
+-- test handling of merge/hash clauses that do not have valid commutators
+--
+
+-- There are not (and should not be) any such operators built into Postgres
+-- that are mergejoinable or hashable but have no commutators; so we leverage
+-- the alias type 'int8alias1' created in this file to conduct the tests.
+-- That's why this test is included here rather than in join.sql.
+
+begin;
+
+create table tbl_nocom(a int8, b int8alias1);
+
+-- check that non-commutable merge clauses do not lead to error
+set enable_hashjoin to off;
+set enable_mergejoin to on;
+explain (costs off)
+select * from tbl_nocom t1 full join tbl_nocom t2 on t2.a = t1.b;
+
+-- check that non-commutable hash clauses do not lead to error
+alter operator = (int8, int8alias1) set (hashes);
+alter operator family integer_ops using hash add
+  operator 1 = (int8, int8alias1);
+create function hashint8alias1(int8alias1) returns int
+  strict immutable language internal as 'hashint8';
+alter operator family integer_ops using hash add
+  function 1 hashint8alias1(int8alias1);
+set enable_hashjoin to on;
+set enable_mergejoin to off;
+explain (costs off)
+select * from tbl_nocom t1 full join tbl_nocom t2 on t2.a = t1.b;
+
+abort;