Inconsistency between try_mergejoin_path and create_mergejoin_plan

From 45e2aa24c3b5d550e8be6ace105f653f6d52e826 Mon Sep 17 00:00:00 2001
From: Richard Guo <guofenglinux@gmail.com>
Date: Wed, 19 Jun 2024 19:57:40 +0900
Subject: [PATCH v2] 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.  So this patch adds
checks to verify the validity of commutators for clauses in the form
"inner op outer" when selecting mergejoin/hash clauses.
---
 src/backend/optimizer/path/joinpath.c | 27 ++++++++++
 src/test/regress/expected/join.out    | 71 +++++++++++++++++++++++++++
 src/test/regress/sql/join.sql         | 58 ++++++++++++++++++++++
 3 files changed, 156 insertions(+)

diff --git a/src/backend/optimizer/path/joinpath.c b/src/backend/optimizer/path/joinpath.c
index 5be8da9e09..bef1944196 100644
--- a/src/backend/optimizer/path/joinpath.c
+++ b/src/backend/optimizer/path/joinpath.c
@@ -24,6 +24,7 @@
 #include "optimizer/pathnode.h"
 #include "optimizer/paths.h"
 #include "optimizer/planmain.h"
+#include "utils/lsyscache.h"
 #include "utils/typcache.h"
 
 /* Hook for plugins to get control in add_paths_to_joinrel() */
@@ -2131,6 +2132,17 @@ 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 will get
+		 * commuted if needed 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.
+		 */
+		if (!restrictinfo->outer_is_left &&
+			!OidIsValid(get_commutator(((OpExpr *) restrictinfo->clause)->opno)))
+			continue;
+
 		hashclauses = lappend(hashclauses, restrictinfo);
 	}
 
@@ -2394,6 +2406,21 @@ 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 will
+		 * get commuted if needed 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.
+		 */
+		if (!restrictinfo->outer_is_left &&
+			!OidIsValid(get_commutator(((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/join.out b/src/test/regress/expected/join.out
index 6b16c3a676..fb2c88d2a6 100644
--- a/src/test/regress/expected/join.out
+++ b/src/test/regress/expected/join.out
@@ -7960,3 +7960,74 @@ SELECT t1.a FROM skip_fetch t1 LEFT JOIN skip_fetch t2 ON t2.a = 1 WHERE t2.a IS
 
 RESET enable_indexonlyscan;
 RESET enable_seqscan;
+--
+-- test handling of merge/hash clauses that do not have valid commutators
+--
+-- Typically there are not any such operators that are mergejoinable or
+-- hashable but have no commutators; so we have to hack things up to create one.
+create type int8nocommutator;
+create function int8nocommutatorin(cstring) returns int8nocommutator
+  strict immutable language internal as 'int8in';
+NOTICE:  return type int8nocommutator is only a shell
+create function int8nocommutatorout(int8nocommutator) returns cstring
+  strict immutable language internal as 'int8out';
+NOTICE:  argument type int8nocommutator is only a shell
+create type int8nocommutator (
+    input = int8nocommutatorin,
+    output = int8nocommutatorout,
+    like = int8
+);
+create function int8nocommutatoreq(int8, int8nocommutator) returns bool
+  strict immutable language internal as 'int8eq';
+create operator = (
+    procedure = int8nocommutatoreq,
+    leftarg = int8, rightarg = int8nocommutator,
+    restrict = eqsel, join = eqjoinsel,
+    hashes, merges
+);
+alter operator family integer_ops using btree add
+  operator 3 = (int8, int8nocommutator);
+create function int8nocommutatorlt(int8nocommutator, int8nocommutator) returns bool
+  strict immutable language internal as 'int8lt';
+create operator < (
+    procedure = int8nocommutatorlt,
+    leftarg = int8nocommutator, rightarg = int8nocommutator
+);
+alter operator family integer_ops using btree add
+  operator 1 < (int8nocommutator, int8nocommutator);
+create function int8nocommutatorcmp(int8, int8nocommutator) returns int
+  strict immutable language internal as 'btint8cmp';
+alter operator family integer_ops using btree add
+  function 1 int8nocommutatorcmp (int8, int8nocommutator);
+create temp table tbl_nocom(a int8, b int8nocommutator);
+-- check that non-commutable merge clauses do not lead to error
+set enable_hashjoin to off;
+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
+reset enable_hashjoin;
+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)
+
+reset enable_mergejoin;
diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql
index 8bfe3b7ba6..2a07d30a1e 100644
--- a/src/test/regress/sql/join.sql
+++ b/src/test/regress/sql/join.sql
@@ -2928,3 +2928,61 @@ SELECT t1.a FROM skip_fetch t1 LEFT JOIN skip_fetch t2 ON t2.a = 1 WHERE t2.a IS
 
 RESET enable_indexonlyscan;
 RESET enable_seqscan;
+
+--
+-- test handling of merge/hash clauses that do not have valid commutators
+--
+
+-- Typically there are not any such operators that are mergejoinable or
+-- hashable but have no commutators; so we have to hack things up to create one.
+
+create type int8nocommutator;
+create function int8nocommutatorin(cstring) returns int8nocommutator
+  strict immutable language internal as 'int8in';
+create function int8nocommutatorout(int8nocommutator) returns cstring
+  strict immutable language internal as 'int8out';
+create type int8nocommutator (
+    input = int8nocommutatorin,
+    output = int8nocommutatorout,
+    like = int8
+);
+
+create function int8nocommutatoreq(int8, int8nocommutator) returns bool
+  strict immutable language internal as 'int8eq';
+create operator = (
+    procedure = int8nocommutatoreq,
+    leftarg = int8, rightarg = int8nocommutator,
+    restrict = eqsel, join = eqjoinsel,
+    hashes, merges
+);
+alter operator family integer_ops using btree add
+  operator 3 = (int8, int8nocommutator);
+
+create function int8nocommutatorlt(int8nocommutator, int8nocommutator) returns bool
+  strict immutable language internal as 'int8lt';
+create operator < (
+    procedure = int8nocommutatorlt,
+    leftarg = int8nocommutator, rightarg = int8nocommutator
+);
+alter operator family integer_ops using btree add
+  operator 1 < (int8nocommutator, int8nocommutator);
+
+create function int8nocommutatorcmp(int8, int8nocommutator) returns int
+  strict immutable language internal as 'btint8cmp';
+alter operator family integer_ops using btree add
+  function 1 int8nocommutatorcmp (int8, int8nocommutator);
+
+create temp table tbl_nocom(a int8, b int8nocommutator);
+
+-- check that non-commutable merge clauses do not lead to error
+set enable_hashjoin to off;
+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
+reset enable_hashjoin;
+set enable_mergejoin to off;
+explain (costs off)
+select * from tbl_nocom t1 full join tbl_nocom t2 on t2.a = t1.b;
+
+reset enable_mergejoin;
-- 
2.43.0

From 9ccf0fd191fadee911e470c277d16fd470fe225b Mon Sep 17 00:00:00 2001
From: Richard Guo <guofenglinux@gmail.com>
Date: Wed, 19 Jun 2024 19:57:40 +0900
Subject: [PATCH v3] 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.
---
 src/backend/optimizer/path/joinpath.c | 27 ++++++++++
 src/test/regress/expected/join.out    | 72 +++++++++++++++++++++++++++
 src/test/regress/sql/join.sql         | 59 ++++++++++++++++++++++
 3 files changed, 158 insertions(+)

diff --git a/src/backend/optimizer/path/joinpath.c b/src/backend/optimizer/path/joinpath.c
index 5be8da9e09..5c8f436367 100644
--- a/src/backend/optimizer/path/joinpath.c
+++ b/src/backend/optimizer/path/joinpath.c
@@ -24,6 +24,7 @@
 #include "optimizer/pathnode.h"
 #include "optimizer/paths.h"
 #include "optimizer/planmain.h"
+#include "utils/lsyscache.h"
 #include "utils/typcache.h"
 
 /* Hook for plugins to get control in add_paths_to_joinrel() */
@@ -2131,6 +2132,17 @@ 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.
+		 */
+		if (!restrictinfo->outer_is_left &&
+			!OidIsValid(get_commutator(((OpExpr *) restrictinfo->clause)->opno)))
+			continue;
+
 		hashclauses = lappend(hashclauses, restrictinfo);
 	}
 
@@ -2394,6 +2406,21 @@ 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.
+		 */
+		if (!restrictinfo->outer_is_left &&
+			!OidIsValid(get_commutator(((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/join.out b/src/test/regress/expected/join.out
index 6b16c3a676..77d1df17d0 100644
--- a/src/test/regress/expected/join.out
+++ b/src/test/regress/expected/join.out
@@ -7960,3 +7960,75 @@ SELECT t1.a FROM skip_fetch t1 LEFT JOIN skip_fetch t2 ON t2.a = 1 WHERE t2.a IS
 
 RESET enable_indexonlyscan;
 RESET enable_seqscan;
+--
+-- 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 have to
+-- hack things up to create one.
+create type int8nocommutator;
+create function int8nocommutatorin(cstring) returns int8nocommutator
+  strict immutable language internal as 'int8in';
+NOTICE:  return type int8nocommutator is only a shell
+create function int8nocommutatorout(int8nocommutator) returns cstring
+  strict immutable language internal as 'int8out';
+NOTICE:  argument type int8nocommutator is only a shell
+create type int8nocommutator (
+    input = int8nocommutatorin,
+    output = int8nocommutatorout,
+    like = int8
+);
+create function int8nocommutatoreq(int8, int8nocommutator) returns bool
+  strict immutable language internal as 'int8eq';
+create operator = (
+    procedure = int8nocommutatoreq,
+    leftarg = int8, rightarg = int8nocommutator,
+    restrict = eqsel, join = eqjoinsel,
+    hashes, merges
+);
+alter operator family integer_ops using btree add
+  operator 3 = (int8, int8nocommutator);
+create function int8nocommutatorlt(int8nocommutator, int8nocommutator) returns bool
+  strict immutable language internal as 'int8lt';
+create operator < (
+    procedure = int8nocommutatorlt,
+    leftarg = int8nocommutator, rightarg = int8nocommutator
+);
+alter operator family integer_ops using btree add
+  operator 1 < (int8nocommutator, int8nocommutator);
+create function int8nocommutatorcmp(int8, int8nocommutator) returns int
+  strict immutable language internal as 'btint8cmp';
+alter operator family integer_ops using btree add
+  function 1 int8nocommutatorcmp (int8, int8nocommutator);
+create temp table tbl_nocom(a int8, b int8nocommutator);
+-- check that non-commutable merge clauses do not lead to error
+set enable_hashjoin to off;
+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
+reset enable_hashjoin;
+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)
+
+reset enable_mergejoin;
diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql
index 8bfe3b7ba6..9405654d54 100644
--- a/src/test/regress/sql/join.sql
+++ b/src/test/regress/sql/join.sql
@@ -2928,3 +2928,62 @@ SELECT t1.a FROM skip_fetch t1 LEFT JOIN skip_fetch t2 ON t2.a = 1 WHERE t2.a IS
 
 RESET enable_indexonlyscan;
 RESET enable_seqscan;
+
+--
+-- 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 have to
+-- hack things up to create one.
+
+create type int8nocommutator;
+create function int8nocommutatorin(cstring) returns int8nocommutator
+  strict immutable language internal as 'int8in';
+create function int8nocommutatorout(int8nocommutator) returns cstring
+  strict immutable language internal as 'int8out';
+create type int8nocommutator (
+    input = int8nocommutatorin,
+    output = int8nocommutatorout,
+    like = int8
+);
+
+create function int8nocommutatoreq(int8, int8nocommutator) returns bool
+  strict immutable language internal as 'int8eq';
+create operator = (
+    procedure = int8nocommutatoreq,
+    leftarg = int8, rightarg = int8nocommutator,
+    restrict = eqsel, join = eqjoinsel,
+    hashes, merges
+);
+alter operator family integer_ops using btree add
+  operator 3 = (int8, int8nocommutator);
+
+create function int8nocommutatorlt(int8nocommutator, int8nocommutator) returns bool
+  strict immutable language internal as 'int8lt';
+create operator < (
+    procedure = int8nocommutatorlt,
+    leftarg = int8nocommutator, rightarg = int8nocommutator
+);
+alter operator family integer_ops using btree add
+  operator 1 < (int8nocommutator, int8nocommutator);
+
+create function int8nocommutatorcmp(int8, int8nocommutator) returns int
+  strict immutable language internal as 'btint8cmp';
+alter operator family integer_ops using btree add
+  function 1 int8nocommutatorcmp (int8, int8nocommutator);
+
+create temp table tbl_nocom(a int8, b int8nocommutator);
+
+-- check that non-commutable merge clauses do not lead to error
+set enable_hashjoin to off;
+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
+reset enable_hashjoin;
+set enable_mergejoin to off;
+explain (costs off)
+select * from tbl_nocom t1 full join tbl_nocom t2 on t2.a = t1.b;
+
+reset enable_mergejoin;
-- 
2.43.0

From 53a39d91f08d169a328d67c906e70f62ff156407 Mon Sep 17 00:00:00 2001
From: Richard Guo <guofenglinux@gmail.com>
Date: Wed, 19 Jun 2024 19:57:40 +0900
Subject: [PATCH v4] 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.
---
 src/backend/optimizer/path/joinpath.c    | 32 ++++++++++++++++++
 src/test/regress/expected/equivclass.out | 42 ++++++++++++++++++++++++
 src/test/regress/sql/equivclass.sql      | 28 ++++++++++++++++
 3 files changed, 102 insertions(+)

diff --git a/src/backend/optimizer/path/joinpath.c b/src/backend/optimizer/path/joinpath.c
index 7a2c20b145..72bdf2f50a 100644
--- 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() */
@@ -2267,6 +2268,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);
 	}
 
@@ -2541,6 +2556,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 126f7047fe..5635ef2141 100644
--- a/src/test/regress/expected/equivclass.out
+++ b/src/test/regress/expected/equivclass.out
@@ -451,3 +451,45 @@ 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);
+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 247b0a3105..87daf6c5f7 100644
--- a/src/test/regress/sql/equivclass.sql
+++ b/src/test/regress/sql/equivclass.sql
@@ -269,3 +269,31 @@ 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);
+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;
-- 
2.43.0

From bc3b2a22b8397a445d3407cf3129c5980d175d9e Mon Sep 17 00:00:00 2001
From: Richard Guo <guofenglinux@gmail.com>
Date: Wed, 19 Jun 2024 19:57:40 +0900
Subject: [PATCH v5] 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.
---
 src/backend/optimizer/path/joinpath.c    | 32 ++++++++++++++++
 src/test/regress/expected/equivclass.out | 48 ++++++++++++++++++++++++
 src/test/regress/sql/equivclass.sql      | 34 +++++++++++++++++
 3 files changed, 114 insertions(+)

diff --git a/src/backend/optimizer/path/joinpath.c b/src/backend/optimizer/path/joinpath.c
index b0e8c94dfc..a244300463 100644
--- 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 126f7047fe..a328164fe0 100644
--- 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 247b0a3105..28ed7910d0 100644
--- 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;
-- 
2.43.0